| Message ID | 20230902100728.3850149-1-liushixin2@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, memcg: expose swapcache stat for memcg v1 | expand |
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
On Sat, Sep 02, 2023 at 06:07:28PM +0800, Liu Shixin 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. Why are we adding new features to the long deprecated cgroup v1?
On Tue, Sep 26, 2023 at 12:30 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Sat, Sep 02, 2023 at 06:07:28PM +0800, Liu Shixin 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. > > Why are we adding new features to the long deprecated cgroup v1? > Technically this is not a new feature, we are just exposing a stat that the kernel is already tracking (for both cgroup v1 and v2). The reason I suggested to expose this for cgroup v1 is because a recent series [1] started using this stat in the kernel to make some reclaim decisions, for both cgroup v1 and v2. Exposing the stat in v1 will make sure no one assumes the stat is only useful for v2 and break the tracking for v1 (which might cause some reclaim regressions then). [1]https://lore.kernel.org/lkml/20230915083417.3190512-1-liushixin2@huawei.com/
diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst index fabaad3fd9c2..fb4abe0dc228 100644 --- a/Documentation/admin-guide/cgroup-v1/memory.rst +++ b/Documentation/admin-guide/cgroup-v1/memory.rst @@ -546,6 +546,7 @@ memory.stat file includes following statistics: event happens each time a page is unaccounted from the cgroup. swap # of bytes of swap usage + swapcached # of bytes of swap cached in memory dirty # of bytes that are waiting to get written back to the disk. writeback # of bytes of file/anon cache that are queued for syncing to disk. diff --git a/mm/memcontrol.c b/mm/memcontrol.c index c465829db92b..619acf479be7 100644 --- 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 }; static const char *const memcg1_stat_names[] = { @@ -4083,6 +4086,9 @@ static const char *const memcg1_stat_names[] = { "workingset_refault_anon", "workingset_refault_file", "swap", +#ifdef CONFIG_SWAP + "swapcached", +#endif }; /* Universal VM events cgroup1 shows, original sort order */
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. Suggested-by: Yosry Ahmed <yosryahmed@google.com> Signed-off-by: Liu Shixin <liushixin2@huawei.com> --- Documentation/admin-guide/cgroup-v1/memory.rst | 1 + mm/memcontrol.c | 6 ++++++ 2 files changed, 7 insertions(+)