On Thu, Mar 18, 2021 at 10:49 PM Johannes Weiner <han...@cmpxchg.org> wrote:
>
> The swapaccounting= commandline option already does very little
> today. To close a trivial containment failure case, the swap ownership
> tracking part of the swap controller has recently become mandatory
> (see commit 2d1c498072de ("mm: memcontrol: make swap tracking an
> integral part of memory control") for details), which makes up the
> majority of the work during swapout, swapin, and the swap slot map.
>
> The only thing left under this flag is the page_counter operations and
> the visibility of the swap control files in the first place, which are
> rather meager savings. There also aren't many scenarios, if any, where
> controlling the memory of a cgroup while allowing it unlimited access
> to a global swap space is a workable resource isolation stragegy.
*strategy
>
> On the other hand, there have been several bugs and confusion around
> the many possible swap controller states (cgroup1 vs cgroup2 behavior,
> memory accounting without swap accounting, memcg runtime disabled).
>
> This puts the maintenance overhead of retaining the toggle above its
> practical benefits. Deprecate it.
>
> Suggested-by: Shakeel Butt <shake...@google.com>
> Signed-off-by: Johannes Weiner <han...@cmpxchg.org>
[...]
>
> static int __init setup_swap_account(char *s)
> {
> - if (!strcmp(s, "1"))
> - cgroup_memory_noswap = false;
> - else if (!strcmp(s, "0"))
> - cgroup_memory_noswap = true;
> - return 1;
> + pr_warn_once("The swapaccount= commandline option is deprecated. "
> + "Please report your usecase to linux...@kvack.org if you
> "
> + "depend on this functionality.\n");
> + return 0;
What's the difference between returning 0 or 1 here?
> }
> __setup("swapaccount=", setup_swap_account);
>
> @@ -7291,27 +7287,13 @@ static struct cftype memsw_files[] = {
> { }, /* terminate */
> };
>
> -/*
> - * If mem_cgroup_swap_init() is implemented as a subsys_initcall()
> - * instead of a core_initcall(), this could mean cgroup_memory_noswap still
> - * remains set to false even when memcg is disabled via
> "cgroup_disable=memory"
> - * boot parameter. This may result in premature OOPS inside
> - * mem_cgroup_get_nr_swap_pages() function in corner cases.
> - */
> static int __init mem_cgroup_swap_init(void)
> {
> - /* No memory control -> no swap control */
> - if (mem_cgroup_disabled())
> - cgroup_memory_noswap = true;
> -
> - if (cgroup_memory_noswap)
> - return 0;
> -
Do we need a mem_cgroup_disabled() check here?
> WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys, swap_files));
> WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys, memsw_files));
>
> return 0;
> }
> -core_initcall(mem_cgroup_swap_init);
> +subsys_initcall(mem_cgroup_swap_init);
>
> #endif /* CONFIG_MEMCG_SWAP */
> --
> 2.30.1
>
