From: Gregory Price <gourry@gourry.net>
To: kaiyang2@cs.cmu.edu
Cc: linux-mm@kvack.org, cgroups@vger.kernel.org,
roman.gushchin@linux.dev, shakeel.butt@linux.dev,
muchun.song@linux.dev, akpm@linux-foundation.org,
mhocko@kernel.org, nehagholkar@meta.com, abhishekd@meta.com,
hannes@cmpxchg.org, weixugc@google.com, rientjes@google.com
Subject: Re: [RFC PATCH 3/4] use memory.low local node protection for local node reclaim
Date: Tue, 15 Oct 2024 17:52:56 -0400 [thread overview]
Message-ID: <Zw7kOJ-LOvQo5PzV@PC2K9PVX.TheFacebook.com> (raw)
In-Reply-To: <20240920221202.1734227-4-kaiyang2@cs.cmu.edu>
On Fri, Sep 20, 2024 at 10:11:50PM +0000, kaiyang2@cs.cmu.edu wrote:
> From: Kaiyang Zhao <kaiyang2@cs.cmu.edu>
>
> When reclaim targets the top-tier node usage by the root memcg,
> apply local memory.low protection instead of global protection.
>
Changelog probably needs a little more context about the intended
affect of this change. What exactly is the implication of this
change compared to applying it against elow?
> Signed-off-by: Kaiyang Zhao <kaiyang2@cs.cmu.edu>
> ---
> include/linux/memcontrol.h | 23 ++++++++++++++---------
> mm/memcontrol.c | 4 ++--
> mm/vmscan.c | 19 ++++++++++++++-----
> 3 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 94aba4498fca..256912b91922 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -586,9 +586,9 @@ static inline bool mem_cgroup_disabled(void)
> static inline void mem_cgroup_protection(struct mem_cgroup *root,
> struct mem_cgroup *memcg,
> unsigned long *min,
> - unsigned long *low)
> + unsigned long *low, unsigned long *locallow)
> {
> - *min = *low = 0;
> + *min = *low = *locallow = 0;
>
"locallow" can be read as "loc allow" or "local low", probably you
want to change all the references to local_low.
Sorry for not saying this on earlier feedback.
> if (mem_cgroup_disabled())
> return;
> @@ -631,10 +631,11 @@ static inline void mem_cgroup_protection(struct mem_cgroup *root,
>
> *min = READ_ONCE(memcg->memory.emin);
> *low = READ_ONCE(memcg->memory.elow);
> + *locallow = READ_ONCE(memcg->memory.elocallow);
> }
>
> void mem_cgroup_calculate_protection(struct mem_cgroup *root,
> - struct mem_cgroup *memcg);
> + struct mem_cgroup *memcg, int is_local);
>
> static inline bool mem_cgroup_unprotected(struct mem_cgroup *target,
> struct mem_cgroup *memcg)
> @@ -651,13 +652,17 @@ static inline bool mem_cgroup_unprotected(struct mem_cgroup *target,
> unsigned long get_cgroup_local_usage(struct mem_cgroup *memcg, bool flush);
>
> static inline bool mem_cgroup_below_low(struct mem_cgroup *target,
> - struct mem_cgroup *memcg)
> + struct mem_cgroup *memcg, int is_local)
> {
> if (mem_cgroup_unprotected(target, memcg))
> return false;
>
> - return READ_ONCE(memcg->memory.elow) >=
> - page_counter_read(&memcg->memory);
> + if (is_local)
> + return READ_ONCE(memcg->memory.elocallow) >=
> + get_cgroup_local_usage(memcg, true);
> + else
> + return READ_ONCE(memcg->memory.elow) >=
> + page_counter_read(&memcg->memory);
Don't need else case here is if block returns.
> }
>
> static inline bool mem_cgroup_below_min(struct mem_cgroup *target,
> @@ -1159,13 +1164,13 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
> static inline void mem_cgroup_protection(struct mem_cgroup *root,
> struct mem_cgroup *memcg,
> unsigned long *min,
> - unsigned long *low)
> + unsigned long *low, unsigned long *locallow)
> {
> *min = *low = 0;
> }
>
> static inline void mem_cgroup_calculate_protection(struct mem_cgroup *root,
> - struct mem_cgroup *memcg)
> + struct mem_cgroup *memcg, int is_local)
> {
> }
>
> @@ -1175,7 +1180,7 @@ static inline bool mem_cgroup_unprotected(struct mem_cgroup *target,
> return true;
> }
> static inline bool mem_cgroup_below_low(struct mem_cgroup *target,
> - struct mem_cgroup *memcg)
> + struct mem_cgroup *memcg, int is_local)
> {
> return false;
> }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d7c5fff12105..61718ba998fe 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4495,7 +4495,7 @@ struct cgroup_subsys memory_cgrp_subsys = {
> * of a top-down tree iteration, not for isolated queries.
> */
> void mem_cgroup_calculate_protection(struct mem_cgroup *root,
> - struct mem_cgroup *memcg)
> + struct mem_cgroup *memcg, int is_local)
> {
> bool recursive_protection =
> cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_RECURSIVE_PROT;
> @@ -4507,7 +4507,7 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root,
> root = root_mem_cgroup;
>
> page_counter_calculate_protection(&root->memory, &memcg->memory,
> - recursive_protection, false);
> + recursive_protection, is_local);
> }
>
> static int charge_memcg(struct folio *folio, struct mem_cgroup *memcg,
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index ce471d686a88..a2681d52fc5f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2377,6 +2377,7 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
> enum scan_balance scan_balance;
> unsigned long ap, fp;
> enum lru_list lru;
> + int is_local = (pgdat->node_id == 0) && root_reclaim(sc);
int should be bool to be more explicit as to what the valid values are.
Should be addressed across the patch set.
>
> /* If we have no swap space, do not bother scanning anon folios. */
> if (!sc->may_swap || !can_reclaim_anon_pages(memcg, pgdat->node_id, sc)) {
> @@ -2457,12 +2458,14 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
> for_each_evictable_lru(lru) {
> bool file = is_file_lru(lru);
> unsigned long lruvec_size;
> - unsigned long low, min;
> + unsigned long low, min, locallow;
> unsigned long scan;
>
> lruvec_size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx);
> mem_cgroup_protection(sc->target_mem_cgroup, memcg,
> - &min, &low);
> + &min, &low, &locallow);
> + if (is_local)
> + low = locallow;
>
> if (min || low) {
> /*
> @@ -2494,7 +2497,12 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
> * again by how much of the total memory used is under
> * hard protection.
> */
> - unsigned long cgroup_size = mem_cgroup_size(memcg);
> + unsigned long cgroup_size;
> +
> + if (is_local)
> + cgroup_size = get_cgroup_local_usage(memcg, true);
> + else
> + cgroup_size = mem_cgroup_size(memcg);
> unsigned long protection;
>
> /* memory.low scaling, make sure we retry before OOM */
> @@ -5869,6 +5877,7 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc)
> };
> struct mem_cgroup_reclaim_cookie *partial = &reclaim;
> struct mem_cgroup *memcg;
> + int is_local = (pgdat->node_id == 0) && root_reclaim(sc);
>
> /*
> * In most cases, direct reclaimers can do partial walks
> @@ -5896,7 +5905,7 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc)
> */
> cond_resched();
>
> - mem_cgroup_calculate_protection(target_memcg, memcg);
> + mem_cgroup_calculate_protection(target_memcg, memcg, is_local);
>
> if (mem_cgroup_below_min(target_memcg, memcg)) {
> /*
> @@ -5904,7 +5913,7 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc)
> * If there is no reclaimable memory, OOM.
> */
> continue;
> - } else if (mem_cgroup_below_low(target_memcg, memcg)) {
> + } else if (mem_cgroup_below_low(target_memcg, memcg, is_local)) {
> /*
> * Soft protection.
> * Respect the protection only as long as
> --
> 2.43.0
>
next prev parent reply other threads:[~2024-10-15 21:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-20 22:11 [RFC PATCH 0/4] memory tiering fairness by per-cgroup control of promotion and demotion kaiyang2
2024-09-20 22:11 ` [RFC PATCH 1/4] Add get_cgroup_local_usage for estimating the top-tier memory usage kaiyang2
2024-09-20 22:11 ` [RFC PATCH 2/4] calculate memory.low for the local node and track its usage kaiyang2
2024-09-22 8:39 ` kernel test robot
2024-10-15 22:05 ` Gregory Price
2024-09-20 22:11 ` [RFC PATCH 3/4] use memory.low local node protection for local node reclaim kaiyang2
2024-10-15 21:52 ` Gregory Price [this message]
2024-09-20 22:11 ` [RFC PATCH 4/4] reduce NUMA balancing scan size of cgroups over their local memory.low kaiyang2
2024-11-08 19:01 ` [RFC PATCH 0/4] memory tiering fairness by per-cgroup control of promotion and demotion kaiyang2
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zw7kOJ-LOvQo5PzV@PC2K9PVX.TheFacebook.com \
--to=gourry@gourry.net \
--cc=abhishekd@meta.com \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=hannes@cmpxchg.org \
--cc=kaiyang2@cs.cmu.edu \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=muchun.song@linux.dev \
--cc=nehagholkar@meta.com \
--cc=rientjes@google.com \
--cc=roman.gushchin@linux.dev \
--cc=shakeel.butt@linux.dev \
--cc=weixugc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox