Hello.
On Sat, Sep 02, 2023 at 06:07:28PM +0800, Liu Shixin <liushixin2@xxxxxxxxxx> 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:
signature.asc
Description: PGP signature
