Re: [PATCH v10 0/6] mm/swap, memcg: Introduce swap tiers for cgroup based swap control

> Hello Yosry, Shakeel,
>
> I have been working through this in detail at the implementation level.
> For now I am adding on/off control of zswap through the tier interface.
>
> > From zswap's perspective, we just need to skip zswap is zswap as a
> > tier is disallowed. Could just be a check in zswap_store() similar to
> > the check if zswap is enabled. I am assuming that if a swap tier is
> > disabled, nothing happens to the existing swapped out pages in this
> > tier, but new pages do not get swapped out to it. This is the same
> > behavior that happens if zswap is disabled at runtime.
>
> This part works as you described, with no real issues.
>
> > From the tiering perspective, we need to accept "zswap" as a possible
> > tier, or maybe creating it as a tier by default if zswap is configured
> > would be better to avoid handling the case where the user doesn't
> > create a tier for zswap. We also need to disallow zswap being the only
> > tier as that combination cannot work without vswap.
>
> I tried to forbid this at the implementation level as you suggested
> , and that is where I ran into trouble.
> A few code paths can end up with zswap as the only
> tier, and they are awkward to handle.  Below is each case and what
> handling it would take:
>
> 1) A write turns off the last device tier while zswap stays on.
>    -> rejected with -EINVAL.  The write does not take effect.
>
> 2) The last device tier is removed via /sys/kernel/mm/swap/tiers.
>    -> the file goes empty, so zswap is not shown either.
>
> 3) A cgroup has zswap and one device tier on, and that tier is removed.
>    -> the cgroup's zswap entry is reset to 0, which the user never
>       asked for.
>
> 4) A cgroup has zswap and device tiers on, and swapoff empties them.
>    -> the cgroup's zswap entry is reset to 0.
>
> So the problem is that in (3) and (4), memory.swap.tiers.max has to
> change on its own independently of what the user wrote
> and it is not like just error handling situation as (1).
>
> There is also a consistency point.  memory.swap.tiers.max already
> accepts a child enabling a tier that its parent has disabled: the write
> succeeds, no error is returned, and the difference is resolved
> internally.  The user's setting is kept as written, and only the
> effective behavior is constrained.  By that logic, accepting a
> zswap-only setting and guaranteeing only that it cannot do anything
> would fit how the interface already behaves.
>
> Having thought it over, I think one of these two directions would be
> better than enforcing the rule as above.
>
> 1. Allow zswap-only in memory.swap.tiers.max.  As you say, it cannot
>    work without vswap, so today the setting does nothing and there is
>    nothing to prevent.  Once vswap/xswap lands it becomes meaningful on its
>    own, with no interface change needed.
>
> 2. Expose memory.swap.tiers.max.effective, like cpuset.  We already
>    track the user-set and the effective-set separately.  Exposing
>    the effective one would show that a zswap-only setting is not in
>    effect, giving the user visibility instead of rewriting what they
>    wrote.  It would also help the parent-off/child-on case, where the
>    child could see from the effective value that the tier is off.
>
> What do you think? or any other ideas?

Hmm yes, seems like enforcing this rule comes with a lot of complexity
for little gain. It's possible today to enable zswap without adding
any swap devices, which would render zswap useless anyway. I suppose
in the same way, we can just do (1) and allow users to disallow all
tiers except zswap for a cgroup, which would effectively disable zswap
in the same way (albeit a bit less clear). We can document this
clearly in the memcg docs.

Shakeel, any objections to this?