linux-kernel - Re: [PATCH] mm, memcg: expose swapcache stat for memcg v1

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]

Date:   Thu, 7 Sep 2023 17:24:52 +0200
From:   Michal Koutný <mkoutny@...e.com>
To:     Liu Shixin <liushixin2@...wei.com>
Cc:     Yosry Ahmed <yosryahmed@...gle.com>, Tejun Heo <tj@...nel.org>,
        Zefan Li <lizefan.x@...edance.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Jonathan Corbet <corbet@....net>,
        Michal Hocko <mhocko@...nel.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <muchun.song@...ux.dev>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        cgroups@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH] mm, memcg: expose swapcache stat for memcg v1

Hello.

On Sat, Sep 02, 2023 at 06:07:28PM +0800, Liu Shixin <liushixin2@...wei.com> wrote:
> Since commit b6038942480e ("mm: memcg: add swapcache stat for memcg v2")
> adds swapcache stat for the cgroup v2, it seems there is no reason to
> hide it in memcg v1. Conversely, with swapcached it is more accurate to
> evaluate the available memory for memcg.

Hm, since the commit b25806dcd3d5 ("mm: memcontrol: deprecate swapaccounting=0 mode") 
do_memsw_account() is synonymous with
!cgroup_subsys_on_dfl(memory_cgrp_subsys) so its uses in
memcg1_stat_format can be simplified.

Would you mind making your patch into a series with (to keep diffstat
low ;-)):

--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4098,8 +4098,6 @@ static void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s)
        for (i = 0; i < ARRAY_SIZE(memcg1_stats); i++) {
                unsigned long nr;
 
-               if (memcg1_stats[i] == MEMCG_SWAP && !do_memsw_account())
-                       continue;
                nr = memcg_page_state_local(memcg, memcg1_stats[i]);
                seq_buf_printf(s, "%s %lu\n", memcg1_stat_names[i],
                           nr * memcg_page_state_unit(memcg1_stats[i]));
@@ -4122,15 +4120,12 @@ static void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s)
        }
        seq_buf_printf(s, "hierarchical_memory_limit %llu\n",
                       (u64)memory * PAGE_SIZE);
-       if (do_memsw_account())
-               seq_buf_printf(s, "hierarchical_memsw_limit %llu\n",
-                              (u64)memsw * PAGE_SIZE);
+       seq_buf_printf(s, "hierarchical_memsw_limit %llu\n",
+                      (u64)memsw * PAGE_SIZE);
 
        for (i = 0; i < ARRAY_SIZE(memcg1_stats); i++) {
                unsigned long nr;
 
-               if (memcg1_stats[i] == MEMCG_SWAP && !do_memsw_account())
-                       continue;
                nr = memcg_page_state(memcg, memcg1_stats[i]);
                seq_buf_printf(s, "total_%s %llu\n", memcg1_stat_names[i],
                           (u64)nr * memcg_page_state_unit(memcg1_stats[i]));


Also,
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4068,6 +4068,9 @@ static const unsigned int memcg1_stats[] = {
>  	WORKINGSET_REFAULT_ANON,
>  	WORKINGSET_REFAULT_FILE,
>  	MEMCG_SWAP,
> +#ifdef CONFIG_SWAP
> +	NR_SWAPCACHE,
> +#endif
>  };

The guard should cover both NR_SWAPCACHE and MEMCG_SWAP or none, no?
(Similarly in memcg1_stat_names.)

Thanks,
Michal


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)