linux-kernel - Re: [PATCH v2 1/2] mm/memcontrol: respect zswap.writeback setting from parent cg too

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJD7tkajuiBDV9Hk8Z+f_-f4ZZf81o4CP3LFLVbfZbrvn4RrUA@mail.gmail.com>
Date: Mon, 19 Aug 2024 12:09:09 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Nhat Pham <nphamcs@...il.com>, Mike Yuan <me@...dnzj.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, cgroups@...r.kernel.org, 
	Johannes Weiner <hannes@...xchg.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	Muchun Song <muchun.song@...ux.dev>, Shakeel Butt <shakeel.butt@...ux.dev>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, Michal Hocko <mhocko@...nel.org>
Subject: Re: [PATCH v2 1/2] mm/memcontrol: respect zswap.writeback setting
 from parent cg too

On Fri, Aug 16, 2024 at 7:44 AM Mike Yuan <me@...dnzj.com> wrote:
>
> Currently, the behavior of zswap.writeback wrt.
> the cgroup hierarchy seems a bit odd. Unlike zswap.max,
> it doesn't honor the value from parent cgroups. This
> surfaced when people tried to globally disable zswap writeback,
> i.e. reserve physical swap space only for hibernation [1] -
> disabling zswap.writeback only for the root cgroup results
> in subcgroups with zswap.writeback=1 still performing writeback.
>
> The inconsistency became more noticeable after I introduced
> the MemoryZSwapWriteback= systemd unit setting [2] for
> controlling the knob. The patch assumed that the kernel would
> enforce the value of parent cgroups. It could probably be
> workarounded from systemd's side, by going up the slice unit
> tree and inheriting the value. Yet I think it's more sensible
> to make it behave consistently with zswap.max and friends.
>
> [1] https://wiki.archlinux.org/title/Power_management/Suspend_and_hibernate#Disable_zswap_writeback_to_use_the_swap_space_only_for_hibernation
> [2] https://github.com/systemd/systemd/pull/31734
>
> Changes in v2:
> - Actually base on latest tree (is_zswap_enabled() -> zswap_is_enabled())
> - Updated Documentation/admin-guide/cgroup-v2.rst to reflect the change
>
> Link to v1: https://lore.kernel.org/linux-kernel/20240814171800.23558-1-me@yhndnzj.com/
>
> Cc: Nhat Pham <nphamcs@...il.com>
> Cc: Yosry Ahmed <yosryahmed@...gle.com>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
>
> Signed-off-by: Mike Yuan <me@...dnzj.com>
> Reviewed-by: Nhat Pham <nphamcs@...il.com>

LGTM,
Acked-by: Yosry Ahmed <yosryahmed@...gle.com>

> ---
>  Documentation/admin-guide/cgroup-v2.rst | 5 ++++-
>  mm/memcontrol.c                         | 9 ++++++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 86311c2907cd..80906cea4264 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1719,7 +1719,10 @@ The following nested keys are defined.
>    memory.zswap.writeback
>         A read-write single value file. The default value is "1". The
>         initial value of the root cgroup is 1, and when a new cgroup is
> -       created, it inherits the current value of its parent.
> +       created, it inherits the current value of its parent. Note that
> +       this setting is hierarchical, i.e. the writeback would be
> +       implicitly disabled for child cgroups if the upper hierarchy
> +       does so.
>
>         When this is set to 0, all swapping attempts to swapping devices
>         are disabled. This included both zswap writebacks, and swapping due
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f29157288b7d..327b2b030639 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5320,7 +5320,14 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size)
>  bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
>  {
>         /* if zswap is disabled, do not block pages going to the swapping device */
> -       return !zswap_is_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
> +       if (!zswap_is_enabled())
> +               return true;

This is orthogonal to this patch, but I just realized that we
completely ignore memory.zswap_writeback if zswap is disabled. This
means that if a cgroup has disabled writeback, then zswap is globally
disabled for some reason, we stop respecting the cgroup knob. I guess
the rationale could be that we want to help get pages out of zswap as
much as possible to honor zswap's disablement? Nhat, did I get that
right?

I feel like it's a little bit odd to be honest, but I don't have a
strong opinion on it. Maybe we should document this behavior better.


> +
> +       for (; memcg; memcg = parent_mem_cgroup(memcg))
> +               if (!READ_ONCE(memcg->zswap_writeback))
> +                       return false;
> +
> +       return true;
>  }
>
>  static u64 zswap_current_read(struct cgroup_subsys_state *css,
>
> base-commit: d07b43284ab356daf7ec5ae1858a16c1c7b6adab
> --
> 2.46.0
>
>