[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45e2c372f59748262b6e4390dc5548f8ebf6c41a.camel@yhndnzj.com>
Date: Tue, 20 Aug 2024 09:38:14 +0000
From: Mike Yuan <me@...dnzj.com>
To: Yosry Ahmed <yosryahmed@...gle.com>, Nhat Pham <nphamcs@...il.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 2024-08-19 at 12:09 -0700, Yosry Ahmed wrote:
> 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?
Hmm, I think the current behavior makes more sense. If zswap is
completely
disabled, it seems intuitive that zswap-related knobs lose their
effect.
> 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.
But clarify this in the documentation certainly sounds good :)
>
> > +
> > + 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
> >
> >
Powered by blists - more mailing lists