commit 3f1509c57b1b ("Revert "mm/vmscan: never demote for memcg
reclaim"") enabled demotion in memcg reclaim, which is the right thing
to do, but introduced a regression in the behavior of
try_to_free_mem_cgroup_pages().
The callers of try_to_free_mem_cgroup_pages() expect it to attempt to
reclaim - not demote - nr_pages from the cgroup. I.e. the memory usage
of the cgroup should reduce by nr_pages. The callers expect
try_to_free_mem_cgroup_pages() to also return the number of pages
reclaimed, not demoted.
However, try_to_free_mem_cgroup_pages() actually unconditionally counts
demoted pages as reclaimed pages. So in practice when it is called it will
often demote nr_pages and return the number of demoted pages to the caller.
Demoted pages don't lower the memcg usage as the caller requested.
I suspect various things work suboptimally on memory systems or don't
work at all due to this:
- memory.high enforcement likely doesn't work (it just demotes nr_pages
instead of lowering the memcg usage by nr_pages).
- try_charge_memcg() will keep retrying the charge while
try_to_free_mem_cgroup_pages() is just demoting pages and not actually
making any room for the charge.
- memory.reclaim has a wonky interface. It advertises to the user it
reclaims the provided amount but it will actually demote that amount.
There may be more effects to this issue.
To fix these issues I propose shrink_folio_list() to only count pages
demoted from inside of sc->nodemask to outside of sc->nodemask as
'reclaimed'.
For callers such as reclaim_high() or try_charge_memcg() that set
sc->nodemask to NULL, try_to_free_mem_cgroup_pages() will try to
actually reclaim nr_pages and return the number of pages reclaimed. No
demoted pages would count towards the nr_pages requirement.
For callers such as memory_reclaim() that set sc->nodemask,
try_to_free_mem_cgroup_pages() will free nr_pages from that nodemask
with either demotion or reclaim.
Tested this change using memory.reclaim interface. With this change,
echo "1m" > memory.reclaim
Will cause freeing of 1m of memory from the cgroup regardless of the
demotions happening inside.
echo "1m nodes=0" > memory.reclaim
Will cause freeing of 1m of node 0 by demotion if a demotion target is
available, and by reclaim if no demotion target is available.
Signed-off-by: Mina Almasry <[email protected]>
---
This is developed on top of mm-unstable largely to test with memory.reclaim
nodes= arg and ensure the fix is compatible with that.
v2:
- Shortened the commit message a bit.
- Fixed issue when demotion falls back to other allowed target nodes returned by
node_get_allowed_targets() as Wei suggested.
Cc: [email protected]
---
include/linux/memory-tiers.h | 7 +++++--
mm/memory-tiers.c | 10 +++++++++-
mm/vmscan.c | 20 +++++++++++++++++---
3 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
index fc9647b1b4f9..f3f359760fd0 100644
--- a/include/linux/memory-tiers.h
+++ b/include/linux/memory-tiers.h
@@ -38,7 +38,8 @@ void init_node_memory_type(int node, struct memory_dev_type *default_type);
void clear_node_memory_type(int node, struct memory_dev_type *memtype);
#ifdef CONFIG_MIGRATION
int next_demotion_node(int node);
-void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets);
+void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets,
+ nodemask_t *demote_from_targets);
bool node_is_toptier(int node);
#else
static inline int next_demotion_node(int node)
@@ -46,7 +47,9 @@ static inline int next_demotion_node(int node)
return NUMA_NO_NODE;
}
-static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
+static inline void node_get_allowed_targets(pg_data_t *pgdat,
+ nodemask_t *targets,
+ nodemask_t *demote_from_targets)
{
*targets = NODE_MASK_NONE;
}
diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
index c734658c6242..7f8f0b5de2b3 100644
--- a/mm/memory-tiers.c
+++ b/mm/memory-tiers.c
@@ -264,7 +264,8 @@ bool node_is_toptier(int node)
return toptier;
}
-void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
+void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets,
+ nodemask_t *demote_from_targets)
{
struct memory_tier *memtier;
@@ -280,6 +281,13 @@ void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
else
*targets = NODE_MASK_NONE;
rcu_read_unlock();
+
+ /*
+ * Exclude the demote_from_targets from the allowed targets if we're
+ * trying to demote from a specific set of nodes.
+ */
+ if (demote_from_targets)
+ nodes_andnot(*targets, *targets, *demote_from_targets);
}
/**
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 2b42ac9ad755..97ca0445b5dc 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1590,7 +1590,8 @@ static struct page *alloc_demote_page(struct page *page, unsigned long private)
* Folios which are not demoted are left on @demote_folios.
*/
static unsigned int demote_folio_list(struct list_head *demote_folios,
- struct pglist_data *pgdat)
+ struct pglist_data *pgdat,
+ nodemask_t *demote_from_nodemask)
{
int target_nid = next_demotion_node(pgdat->node_id);
unsigned int nr_succeeded;
@@ -1614,7 +1615,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
if (target_nid == NUMA_NO_NODE)
return 0;
- node_get_allowed_targets(pgdat, &allowed_mask);
+ node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
/* Demotion ignores all cpuset and mempolicy settings */
migrate_pages(demote_folios, alloc_demote_page, NULL,
@@ -1653,6 +1654,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
LIST_HEAD(free_folios);
LIST_HEAD(demote_folios);
unsigned int nr_reclaimed = 0;
+ unsigned int nr_demoted = 0;
unsigned int pgactivate = 0;
bool do_demote_pass;
struct swap_iocb *plug = NULL;
@@ -2085,7 +2087,19 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
/* 'folio_list' is always empty here */
/* Migrate folios selected for demotion */
- nr_reclaimed += demote_folio_list(&demote_folios, pgdat);
+ nr_demoted = demote_folio_list(&demote_folios, pgdat, sc->nodemask);
+
+ /*
+ * Only count demoted folios as reclaimed if the caller has requested
+ * demotion from a specific nodemask. In this case pages inside the
+ * noedmask have been demoted to outside the nodemask and we can count
+ * these pages as reclaimed. If no nodemask is passed, then the caller
+ * is requesting reclaim from all memory, which should not count
+ * demoted pages.
+ */
+ if (sc->nodemask)
+ nr_reclaimed += nr_demoted;
+
/* Folios that could not be demoted are still in @demote_folios */
if (!list_empty(&demote_folios)) {
/* Folios which weren't demoted go back on @folio_list */
--
2.39.0.rc0.267.gcb52ba06e7-goog
Hi Mina,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on akpm-mm/mm-everything]
url: https://github.com/intel-lab-lkp/linux/commits/Mina-Almasry/mm-Fix-memcg-reclaim-on-memory-tiered-systems/20221204-173146
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20221204093008.2620459-1-almasrymina%40google.com
patch subject: [PATCH v2] [mm-unstable] mm: Fix memcg reclaim on memory tiered systems
config: alpha-buildonly-randconfig-r005-20221204
compiler: alpha-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/332f41fcb1b1d6bc7dafd40e8c1e10a1a0952849
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mina-Almasry/mm-Fix-memcg-reclaim-on-memory-tiered-systems/20221204-173146
git checkout 332f41fcb1b1d6bc7dafd40e8c1e10a1a0952849
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
mm/vmscan.c: In function 'demote_folio_list':
>> mm/vmscan.c:1618:9: error: too many arguments to function 'node_get_allowed_targets'
1618 | node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
| ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from mm/vmscan.c:46:
include/linux/memory-tiers.h:94:20: note: declared here
94 | static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
| ^~~~~~~~~~~~~~~~~~~~~~~~
vim +/node_get_allowed_targets +1618 mm/vmscan.c
1587
1588 /*
1589 * Take folios on @demote_folios and attempt to demote them to another node.
1590 * Folios which are not demoted are left on @demote_folios.
1591 */
1592 static unsigned int demote_folio_list(struct list_head *demote_folios,
1593 struct pglist_data *pgdat,
1594 nodemask_t *demote_from_nodemask)
1595 {
1596 int target_nid = next_demotion_node(pgdat->node_id);
1597 unsigned int nr_succeeded;
1598 nodemask_t allowed_mask;
1599
1600 struct migration_target_control mtc = {
1601 /*
1602 * Allocate from 'node', or fail quickly and quietly.
1603 * When this happens, 'page' will likely just be discarded
1604 * instead of migrated.
1605 */
1606 .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) | __GFP_NOWARN |
1607 __GFP_NOMEMALLOC | GFP_NOWAIT,
1608 .nid = target_nid,
1609 .nmask = &allowed_mask
1610 };
1611
1612 if (list_empty(demote_folios))
1613 return 0;
1614
1615 if (target_nid == NUMA_NO_NODE)
1616 return 0;
1617
> 1618 node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
1619
1620 /* Demotion ignores all cpuset and mempolicy settings */
1621 migrate_pages(demote_folios, alloc_demote_page, NULL,
1622 (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION,
1623 &nr_succeeded);
1624
1625 __count_vm_events(PGDEMOTE_KSWAPD + reclaimer_offset(), nr_succeeded);
1626
1627 return nr_succeeded;
1628 }
1629
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Sun, Dec 4, 2022 at 2:32 AM kernel test robot <[email protected]> wrote:
>
> Hi Mina,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on akpm-mm/mm-everything]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Mina-Almasry/mm-Fix-memcg-reclaim-on-memory-tiered-systems/20221204-173146
> base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link: https://lore.kernel.org/r/20221204093008.2620459-1-almasrymina%40google.com
> patch subject: [PATCH v2] [mm-unstable] mm: Fix memcg reclaim on memory tiered systems
> config: alpha-buildonly-randconfig-r005-20221204
> compiler: alpha-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/332f41fcb1b1d6bc7dafd40e8c1e10a1a0952849
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Mina-Almasry/mm-Fix-memcg-reclaim-on-memory-tiered-systems/20221204-173146
> git checkout 332f41fcb1b1d6bc7dafd40e8c1e10a1a0952849
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
>
> All errors (new ones prefixed by >>):
>
> mm/vmscan.c: In function 'demote_folio_list':
> >> mm/vmscan.c:1618:9: error: too many arguments to function 'node_get_allowed_targets'
> 1618 | node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
> | ^~~~~~~~~~~~~~~~~~~~~~~~
> In file included from mm/vmscan.c:46:
> include/linux/memory-tiers.h:94:20: note: declared here
> 94 | static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
I missed updating this function signature. Will fix it in v3.
>
> vim +/node_get_allowed_targets +1618 mm/vmscan.c
>
> 1587
> 1588 /*
> 1589 * Take folios on @demote_folios and attempt to demote them to another node.
> 1590 * Folios which are not demoted are left on @demote_folios.
> 1591 */
> 1592 static unsigned int demote_folio_list(struct list_head *demote_folios,
> 1593 struct pglist_data *pgdat,
> 1594 nodemask_t *demote_from_nodemask)
> 1595 {
> 1596 int target_nid = next_demotion_node(pgdat->node_id);
> 1597 unsigned int nr_succeeded;
> 1598 nodemask_t allowed_mask;
> 1599
> 1600 struct migration_target_control mtc = {
> 1601 /*
> 1602 * Allocate from 'node', or fail quickly and quietly.
> 1603 * When this happens, 'page' will likely just be discarded
> 1604 * instead of migrated.
> 1605 */
> 1606 .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) | __GFP_NOWARN |
> 1607 __GFP_NOMEMALLOC | GFP_NOWAIT,
> 1608 .nid = target_nid,
> 1609 .nmask = &allowed_mask
> 1610 };
> 1611
> 1612 if (list_empty(demote_folios))
> 1613 return 0;
> 1614
> 1615 if (target_nid == NUMA_NO_NODE)
> 1616 return 0;
> 1617
> > 1618 node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
> 1619
> 1620 /* Demotion ignores all cpuset and mempolicy settings */
> 1621 migrate_pages(demote_folios, alloc_demote_page, NULL,
> 1622 (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION,
> 1623 &nr_succeeded);
> 1624
> 1625 __count_vm_events(PGDEMOTE_KSWAPD + reclaimer_offset(), nr_succeeded);
> 1626
> 1627 return nr_succeeded;
> 1628 }
> 1629
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
Hi Mina,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on akpm-mm/mm-everything]
url: https://github.com/intel-lab-lkp/linux/commits/Mina-Almasry/mm-Fix-memcg-reclaim-on-memory-tiered-systems/20221204-173146
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20221204093008.2620459-1-almasrymina%40google.com
patch subject: [PATCH v2] [mm-unstable] mm: Fix memcg reclaim on memory tiered systems
config: s390-randconfig-r013-20221204
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 6e4cea55f0d1104408b26ac574566a0e4de48036)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/332f41fcb1b1d6bc7dafd40e8c1e10a1a0952849
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mina-Almasry/mm-Fix-memcg-reclaim-on-memory-tiered-systems/20221204-173146
git checkout 332f41fcb1b1d6bc7dafd40e8c1e10a1a0952849
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> mm/vmscan.c:1618:49: error: too many arguments to function call, expected 2, have 3
node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
~~~~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~
include/linux/memory-tiers.h:94:20: note: 'node_get_allowed_targets' declared here
static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
^
1 error generated.
vim +1618 mm/vmscan.c
1587
1588 /*
1589 * Take folios on @demote_folios and attempt to demote them to another node.
1590 * Folios which are not demoted are left on @demote_folios.
1591 */
1592 static unsigned int demote_folio_list(struct list_head *demote_folios,
1593 struct pglist_data *pgdat,
1594 nodemask_t *demote_from_nodemask)
1595 {
1596 int target_nid = next_demotion_node(pgdat->node_id);
1597 unsigned int nr_succeeded;
1598 nodemask_t allowed_mask;
1599
1600 struct migration_target_control mtc = {
1601 /*
1602 * Allocate from 'node', or fail quickly and quietly.
1603 * When this happens, 'page' will likely just be discarded
1604 * instead of migrated.
1605 */
1606 .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) | __GFP_NOWARN |
1607 __GFP_NOMEMALLOC | GFP_NOWAIT,
1608 .nid = target_nid,
1609 .nmask = &allowed_mask
1610 };
1611
1612 if (list_empty(demote_folios))
1613 return 0;
1614
1615 if (target_nid == NUMA_NO_NODE)
1616 return 0;
1617
> 1618 node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
1619
1620 /* Demotion ignores all cpuset and mempolicy settings */
1621 migrate_pages(demote_folios, alloc_demote_page, NULL,
1622 (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION,
1623 &nr_succeeded);
1624
1625 __count_vm_events(PGDEMOTE_KSWAPD + reclaimer_offset(), nr_succeeded);
1626
1627 return nr_succeeded;
1628 }
1629
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Mina Almasry <[email protected]> writes:
> commit 3f1509c57b1b ("Revert "mm/vmscan: never demote for memcg
> reclaim"") enabled demotion in memcg reclaim, which is the right thing
> to do, but introduced a regression in the behavior of
> try_to_free_mem_cgroup_pages().
>
> The callers of try_to_free_mem_cgroup_pages() expect it to attempt to
> reclaim - not demote - nr_pages from the cgroup. I.e. the memory usage
> of the cgroup should reduce by nr_pages. The callers expect
> try_to_free_mem_cgroup_pages() to also return the number of pages
> reclaimed, not demoted.
>
> However, try_to_free_mem_cgroup_pages() actually unconditionally counts
> demoted pages as reclaimed pages. So in practice when it is called it will
> often demote nr_pages and return the number of demoted pages to the caller.
> Demoted pages don't lower the memcg usage as the caller requested.
>
> I suspect various things work suboptimally on memory systems or don't
> work at all due to this:
>
> - memory.high enforcement likely doesn't work (it just demotes nr_pages
> instead of lowering the memcg usage by nr_pages).
> - try_charge_memcg() will keep retrying the charge while
> try_to_free_mem_cgroup_pages() is just demoting pages and not actually
> making any room for the charge.
> - memory.reclaim has a wonky interface. It advertises to the user it
> reclaims the provided amount but it will actually demote that amount.
>
> There may be more effects to this issue.
>
> To fix these issues I propose shrink_folio_list() to only count pages
> demoted from inside of sc->nodemask to outside of sc->nodemask as
> 'reclaimed'.
>
> For callers such as reclaim_high() or try_charge_memcg() that set
> sc->nodemask to NULL, try_to_free_mem_cgroup_pages() will try to
> actually reclaim nr_pages and return the number of pages reclaimed. No
> demoted pages would count towards the nr_pages requirement.
>
> For callers such as memory_reclaim() that set sc->nodemask,
> try_to_free_mem_cgroup_pages() will free nr_pages from that nodemask
> with either demotion or reclaim.
>
> Tested this change using memory.reclaim interface. With this change,
>
> echo "1m" > memory.reclaim
>
> Will cause freeing of 1m of memory from the cgroup regardless of the
> demotions happening inside.
>
> echo "1m nodes=0" > memory.reclaim
>
> Will cause freeing of 1m of node 0 by demotion if a demotion target is
> available, and by reclaim if no demotion target is available.
>
> Signed-off-by: Mina Almasry <[email protected]>
>
> ---
>
> This is developed on top of mm-unstable largely to test with memory.reclaim
> nodes= arg and ensure the fix is compatible with that.
>
> v2:
> - Shortened the commit message a bit.
> - Fixed issue when demotion falls back to other allowed target nodes returned by
> node_get_allowed_targets() as Wei suggested.
>
> Cc: [email protected]
> ---
> include/linux/memory-tiers.h | 7 +++++--
> mm/memory-tiers.c | 10 +++++++++-
> mm/vmscan.c | 20 +++++++++++++++++---
> 3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
> index fc9647b1b4f9..f3f359760fd0 100644
> --- a/include/linux/memory-tiers.h
> +++ b/include/linux/memory-tiers.h
> @@ -38,7 +38,8 @@ void init_node_memory_type(int node, struct memory_dev_type *default_type);
> void clear_node_memory_type(int node, struct memory_dev_type *memtype);
> #ifdef CONFIG_MIGRATION
> int next_demotion_node(int node);
> -void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets);
> +void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets,
> + nodemask_t *demote_from_targets);
> bool node_is_toptier(int node);
> #else
> static inline int next_demotion_node(int node)
> @@ -46,7 +47,9 @@ static inline int next_demotion_node(int node)
> return NUMA_NO_NODE;
> }
>
> -static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> +static inline void node_get_allowed_targets(pg_data_t *pgdat,
> + nodemask_t *targets,
> + nodemask_t *demote_from_targets)
> {
> *targets = NODE_MASK_NONE;
> }
> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
> index c734658c6242..7f8f0b5de2b3 100644
> --- a/mm/memory-tiers.c
> +++ b/mm/memory-tiers.c
> @@ -264,7 +264,8 @@ bool node_is_toptier(int node)
> return toptier;
> }
>
> -void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> +void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets,
> + nodemask_t *demote_from_targets)
> {
> struct memory_tier *memtier;
>
> @@ -280,6 +281,13 @@ void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> else
> *targets = NODE_MASK_NONE;
> rcu_read_unlock();
> +
> + /*
> + * Exclude the demote_from_targets from the allowed targets if we're
> + * trying to demote from a specific set of nodes.
> + */
> + if (demote_from_targets)
> + nodes_andnot(*targets, *targets, *demote_from_targets);
> }
Will this cause demotion to not work when we have memory policy like
MPOL_BIND with nodemask including demotion targets?
>
> /**
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2b42ac9ad755..97ca0445b5dc 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1590,7 +1590,8 @@ static struct page *alloc_demote_page(struct page *page, unsigned long private)
> * Folios which are not demoted are left on @demote_folios.
> */
> static unsigned int demote_folio_list(struct list_head *demote_folios,
> - struct pglist_data *pgdat)
> + struct pglist_data *pgdat,
> + nodemask_t *demote_from_nodemask)
> {
> int target_nid = next_demotion_node(pgdat->node_id);
> unsigned int nr_succeeded;
> @@ -1614,7 +1615,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
> if (target_nid == NUMA_NO_NODE)
> return 0;
>
> - node_get_allowed_targets(pgdat, &allowed_mask);
> + node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
>
> /* Demotion ignores all cpuset and mempolicy settings */
> migrate_pages(demote_folios, alloc_demote_page, NULL,
> @@ -1653,6 +1654,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
> LIST_HEAD(free_folios);
> LIST_HEAD(demote_folios);
> unsigned int nr_reclaimed = 0;
> + unsigned int nr_demoted = 0;
> unsigned int pgactivate = 0;
> bool do_demote_pass;
> struct swap_iocb *plug = NULL;
> @@ -2085,7 +2087,19 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
> /* 'folio_list' is always empty here */
>
> /* Migrate folios selected for demotion */
> - nr_reclaimed += demote_folio_list(&demote_folios, pgdat);
> + nr_demoted = demote_folio_list(&demote_folios, pgdat, sc->nodemask);
> +
> + /*
> + * Only count demoted folios as reclaimed if the caller has requested
> + * demotion from a specific nodemask. In this case pages inside the
> + * noedmask have been demoted to outside the nodemask and we can count
> + * these pages as reclaimed. If no nodemask is passed, then the caller
> + * is requesting reclaim from all memory, which should not count
> + * demoted pages.
> + */
> + if (sc->nodemask)
> + nr_reclaimed += nr_demoted;
> +
> /* Folios that could not be demoted are still in @demote_folios */
> if (!list_empty(&demote_folios)) {
> /* Folios which weren't demoted go back on @folio_list */
> --
> 2.39.0.rc0.267.gcb52ba06e7-goog
On Wed, Dec 7, 2022 at 12:07 AM Aneesh Kumar K.V
<[email protected]> wrote:
>
> Mina Almasry <[email protected]> writes:
>
> > commit 3f1509c57b1b ("Revert "mm/vmscan: never demote for memcg
> > reclaim"") enabled demotion in memcg reclaim, which is the right thing
> > to do, but introduced a regression in the behavior of
> > try_to_free_mem_cgroup_pages().
> >
> > The callers of try_to_free_mem_cgroup_pages() expect it to attempt to
> > reclaim - not demote - nr_pages from the cgroup. I.e. the memory usage
> > of the cgroup should reduce by nr_pages. The callers expect
> > try_to_free_mem_cgroup_pages() to also return the number of pages
> > reclaimed, not demoted.
> >
> > However, try_to_free_mem_cgroup_pages() actually unconditionally counts
> > demoted pages as reclaimed pages. So in practice when it is called it will
> > often demote nr_pages and return the number of demoted pages to the caller.
> > Demoted pages don't lower the memcg usage as the caller requested.
> >
> > I suspect various things work suboptimally on memory systems or don't
> > work at all due to this:
> >
> > - memory.high enforcement likely doesn't work (it just demotes nr_pages
> > instead of lowering the memcg usage by nr_pages).
> > - try_charge_memcg() will keep retrying the charge while
> > try_to_free_mem_cgroup_pages() is just demoting pages and not actually
> > making any room for the charge.
> > - memory.reclaim has a wonky interface. It advertises to the user it
> > reclaims the provided amount but it will actually demote that amount.
> >
> > There may be more effects to this issue.
> >
> > To fix these issues I propose shrink_folio_list() to only count pages
> > demoted from inside of sc->nodemask to outside of sc->nodemask as
> > 'reclaimed'.
> >
> > For callers such as reclaim_high() or try_charge_memcg() that set
> > sc->nodemask to NULL, try_to_free_mem_cgroup_pages() will try to
> > actually reclaim nr_pages and return the number of pages reclaimed. No
> > demoted pages would count towards the nr_pages requirement.
> >
> > For callers such as memory_reclaim() that set sc->nodemask,
> > try_to_free_mem_cgroup_pages() will free nr_pages from that nodemask
> > with either demotion or reclaim.
> >
> > Tested this change using memory.reclaim interface. With this change,
> >
> > echo "1m" > memory.reclaim
> >
> > Will cause freeing of 1m of memory from the cgroup regardless of the
> > demotions happening inside.
> >
> > echo "1m nodes=0" > memory.reclaim
> >
> > Will cause freeing of 1m of node 0 by demotion if a demotion target is
> > available, and by reclaim if no demotion target is available.
> >
> > Signed-off-by: Mina Almasry <[email protected]>
> >
> > ---
> >
> > This is developed on top of mm-unstable largely to test with memory.reclaim
> > nodes= arg and ensure the fix is compatible with that.
> >
> > v2:
> > - Shortened the commit message a bit.
> > - Fixed issue when demotion falls back to other allowed target nodes returned by
> > node_get_allowed_targets() as Wei suggested.
> >
> > Cc: [email protected]
> > ---
> > include/linux/memory-tiers.h | 7 +++++--
> > mm/memory-tiers.c | 10 +++++++++-
> > mm/vmscan.c | 20 +++++++++++++++++---
> > 3 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
> > index fc9647b1b4f9..f3f359760fd0 100644
> > --- a/include/linux/memory-tiers.h
> > +++ b/include/linux/memory-tiers.h
> > @@ -38,7 +38,8 @@ void init_node_memory_type(int node, struct memory_dev_type *default_type);
> > void clear_node_memory_type(int node, struct memory_dev_type *memtype);
> > #ifdef CONFIG_MIGRATION
> > int next_demotion_node(int node);
> > -void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets);
> > +void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets,
> > + nodemask_t *demote_from_targets);
> > bool node_is_toptier(int node);
> > #else
> > static inline int next_demotion_node(int node)
> > @@ -46,7 +47,9 @@ static inline int next_demotion_node(int node)
> > return NUMA_NO_NODE;
> > }
> >
> > -static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> > +static inline void node_get_allowed_targets(pg_data_t *pgdat,
> > + nodemask_t *targets,
> > + nodemask_t *demote_from_targets)
> > {
> > *targets = NODE_MASK_NONE;
> > }
> > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
> > index c734658c6242..7f8f0b5de2b3 100644
> > --- a/mm/memory-tiers.c
> > +++ b/mm/memory-tiers.c
> > @@ -264,7 +264,8 @@ bool node_is_toptier(int node)
> > return toptier;
> > }
> >
> > -void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> > +void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets,
> > + nodemask_t *demote_from_targets)
> > {
> > struct memory_tier *memtier;
> >
> > @@ -280,6 +281,13 @@ void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> > else
> > *targets = NODE_MASK_NONE;
> > rcu_read_unlock();
> > +
> > + /*
> > + * Exclude the demote_from_targets from the allowed targets if we're
> > + * trying to demote from a specific set of nodes.
> > + */
> > + if (demote_from_targets)
> > + nodes_andnot(*targets, *targets, *demote_from_targets);
> > }
>
> Will this cause demotion to not work when we have memory policy like
> MPOL_BIND with nodemask including demotion targets?
>
Hi Aneesh,
You may want to review v3 of this patch that removed this bit:
https://lore.kernel.org/linux-mm/[email protected]/T/#t
To answer your question though, it will disable demotion between the
MPOL_BIND nodes I think, yes. That may be another reason not to do
this (it's already removed in v3).
>
> >
> > /**
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 2b42ac9ad755..97ca0445b5dc 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1590,7 +1590,8 @@ static struct page *alloc_demote_page(struct page *page, unsigned long private)
> > * Folios which are not demoted are left on @demote_folios.
> > */
> > static unsigned int demote_folio_list(struct list_head *demote_folios,
> > - struct pglist_data *pgdat)
> > + struct pglist_data *pgdat,
> > + nodemask_t *demote_from_nodemask)
> > {
> > int target_nid = next_demotion_node(pgdat->node_id);
> > unsigned int nr_succeeded;
> > @@ -1614,7 +1615,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
> > if (target_nid == NUMA_NO_NODE)
> > return 0;
> >
> > - node_get_allowed_targets(pgdat, &allowed_mask);
> > + node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask);
> >
> > /* Demotion ignores all cpuset and mempolicy settings */
> > migrate_pages(demote_folios, alloc_demote_page, NULL,
> > @@ -1653,6 +1654,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
> > LIST_HEAD(free_folios);
> > LIST_HEAD(demote_folios);
> > unsigned int nr_reclaimed = 0;
> > + unsigned int nr_demoted = 0;
> > unsigned int pgactivate = 0;
> > bool do_demote_pass;
> > struct swap_iocb *plug = NULL;
> > @@ -2085,7 +2087,19 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
> > /* 'folio_list' is always empty here */
> >
> > /* Migrate folios selected for demotion */
> > - nr_reclaimed += demote_folio_list(&demote_folios, pgdat);
> > + nr_demoted = demote_folio_list(&demote_folios, pgdat, sc->nodemask);
> > +
> > + /*
> > + * Only count demoted folios as reclaimed if the caller has requested
> > + * demotion from a specific nodemask. In this case pages inside the
> > + * noedmask have been demoted to outside the nodemask and we can count
> > + * these pages as reclaimed. If no nodemask is passed, then the caller
> > + * is requesting reclaim from all memory, which should not count
> > + * demoted pages.
> > + */
> > + if (sc->nodemask)
> > + nr_reclaimed += nr_demoted;
> > +
> > /* Folios that could not be demoted are still in @demote_folios */
> > if (!list_empty(&demote_folios)) {
> > /* Folios which weren't demoted go back on @folio_list */
> > --
> > 2.39.0.rc0.267.gcb52ba06e7-goog