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

["Michal Koutný" <mkoutny@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

Hello.

On Sat, Sep 02, 2023 at 06:07:28PM +0800, Liu Shixin <liushixin2@huawei.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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]