On Wed, Dec 7, 2022 at 12:07 AM Aneesh Kumar K.V
<aneesh.kumar@xxxxxxxxxxxxx> wrote:
>
> Mina Almasry <almasrymina@xxxxxxxxxx> 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 <almasrymina@xxxxxxxxxx>
> >
> > ---
> >
> > 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: weixugc@xxxxxxxxxx
> > ---
> > 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/202212070124.VxwbfKCK-lkp@xxxxxxxxx/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
