MIME-Version: 1.0
References: <20230202155626.1829121-1-hannes@cmpxchg.org>
In-Reply-To: <20230202155626.1829121-1-hannes@cmpxchg.org>
From:   Yosry Ahmed <yosryahmed@google.com>
Date:   Thu, 2 Feb 2023 10:30:40 -0800
Message-ID: <CAJD7tkaCpD0LpzdA+NsZj2WK=iQCLn7RS9qc7K53Qonxhp4TgA@mail.gmail.com>
Subject: Re: [RFC PATCH] mm: memcontrol: don't account swap failures not due
 to cgroup limits
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Michal Hocko <mhocko@suse.com>, Shakeel Butt <shakeelb@google.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Tejun Heo <tj@kernel.org>, linux-mm@kvack.org,
        cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
        Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 2, 2023 at 7:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> Christian reports the following situation in a cgroup that doesn't
> have memory.swap.max configured:
>
>   $ cat memory.swap.events
>   high 0
>   max 0
>   fail 6218
>
> Upon closer examination, this is an ARM64 machine that doesn't support
> swapping out THPs. In that case, the first get_swap_page() fails, and
> the kernel falls back to splitting the THP and swapping the 4k
> constituents one by one. /proc/vmstat confirms this with a high rate
> of thp_swpout_fallback events.
>
> While the behavior can ultimately be explained, it's unexpected and
> confusing. I see three choices how to address this:
>
> a) Specifically exlude THP fallbacks from being counted, as the
>    failure is transient and the memory is ultimately swapped.
>
>    Arguably, though, the user would like to know if their cgroup's
>    swap limit is causing high rates of THP splitting during swapout.

We have the option to add THP_SWPOUT_FALLBACK (and THP_SWPOUT for
completeness) to memcg events for this if/when a use case arises,
right?

>
> b) Only count cgroup swap events when they are actually due to a
>    cgroup's own limit. Exclude failures that are due to physical swap
>    shortage or other system-level conditions (like !THP_SWAP). Also
>    count them at the level where the limit is configured, which may be
>    above the local cgroup that holds the page-to-be-swapped.
>
>    This is in line with how memory.swap.high, memory.high and
>    memory.max events are counted.
>
>    However, it's a change in documented behavior.

This option makes sense to me, but I can't speak to the change of
documented behavior. However, looking at the code, it seems like if we do this
the "max" & "fail" counters become effectively the same. "fail" would
not provide much value then.

I wonder if it makes sense to have both, and clarify that "fail" -
"max" would be non-limit based failures (e.g. ran out of swap space),
or would this cause confusion as to whether those non-limit failures
were transient (THP fallback) or eventual?


>
> c) Leave it as is. The documentation says system-level events are
>    counted, so stick to that.
>
>    This is the conservative option, but isn't very user friendly.
>    Cgroup events are usually due to a local control choice made by the
>    user. Mixing in events that are beyond the user's control makes it
>    difficult to id root causes and configure the system properly.
>
> Implement option b).
>
> Reported-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  Documentation/admin-guide/cgroup-v2.rst |  6 +++---
>  mm/memcontrol.c                         | 12 +++++-------
>  mm/swap_slots.c                         |  2 +-
>  3 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index c8ae7c897f14..a8ffb89a4169 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1605,9 +1605,9 @@ PAGE_SIZE multiple when read back.
>                 failed.
>
>           fail
> -               The number of times swap allocation failed either
> -               because of running out of swap system-wide or max
> -               limit.
> +
> +               The number of times swap allocation failed because of
> +               the max limit.
>
>         When reduced under the current usage, the existing swap
>         entries are reclaimed gradually and the swap usage may stay
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ab457f0394ab..c2a6206ce84b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7470,17 +7470,15 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry)
>         if (!memcg)
>                 return 0;
>
> -       if (!entry.val) {
> -               memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
> -               return 0;
> -       }
> -
>         memcg = mem_cgroup_id_get_online(memcg);
>
>         if (!mem_cgroup_is_root(memcg) &&
>             !page_counter_try_charge(&memcg->swap, nr_pages, &counter)) {
> -               memcg_memory_event(memcg, MEMCG_SWAP_MAX);
> -               memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
> +               struct mem_cgroup *swap_over_limit;
> +
> +               swap_over_limit = mem_cgroup_from_counter(counter, swap);
> +               memcg_memory_event(swap_over_limit, MEMCG_SWAP_MAX);
> +               memcg_memory_event(swap_over_limit, MEMCG_SWAP_FAIL);
>                 mem_cgroup_id_put(memcg);
>                 return -ENOMEM;
>         }
> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> index 0bec1f705f8e..66076bd60e2b 100644
> --- a/mm/swap_slots.c
> +++ b/mm/swap_slots.c
> @@ -342,7 +342,7 @@ swp_entry_t folio_alloc_swap(struct folio *folio)
>
>         get_swap_pages(1, &entry, 1);
>  out:
> -       if (mem_cgroup_try_charge_swap(folio, entry)) {
> +       if (entry.val && mem_cgroup_try_charge_swap(folio, entry) < 0) {
>                 put_swap_folio(folio, entry);
>                 entry.val = 0;
>         }
> --
> 2.39.1
>
>