Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
To:     Mina Almasry <almasrymina@google.com>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Shakeel Butt <shakeelb@google.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Huang Ying <ying.huang@intel.com>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        Yosry Ahmed <yosryahmed@google.com>, weixugc@google.com,
        fvdl@google.com, Mina Almasry <almasrymina@google.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v2] [mm-unstable] mm: Fix memcg reclaim on memory tiered
 systems
In-Reply-To: <20221204093008.2620459-1-almasrymina@google.com>
References: <20221204093008.2620459-1-almasrymina@google.com>
Date:   Wed, 07 Dec 2022 13:37:40 +0530
Message-ID: <87k033eiwj.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Mina Almasry <almasrymina@google.com> 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@google.com>
>
> ---
>
> 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@google.com
> ---
>  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