Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 6 Jun 2022 14:03:56 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Qi Zheng <zhengqi.arch@bytedance.com>
Cc:     hannes@cmpxchg.org, roman.gushchin@linux.dev, shakeelb@google.com,
        songmuchun@bytedance.com, akpm@linux-foundation.org,
        corbet@lwn.net, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org
Subject: Re: [PATCH v2] mm: memcontrol: add {pgscan,pgsteal}_{kswapd,direct}
 items in memory.stat of cgroup v2
Message-ID: <Yp3tLNi0wybMw7La@dhcp22.suse.cz>
References: <20220604082209.55174-1-zhengqi.arch@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220604082209.55174-1-zhengqi.arch@bytedance.com>
Precedence: bulk

On Sat 04-06-22 16:22:09, Qi Zheng wrote:
> There are already statistics of {pgscan,pgsteal}_kswapd and
> {pgscan,pgsteal}_direct of memcg event here, but now only the
> sum of the two is displayed in memory.stat of cgroup v2.
> 
> In order to obtain more accurate information during monitoring
> and debugging, and to align with the display in /proc/vmstat,
> it better to display {pgscan,pgsteal}_kswapd and
> {pgscan,pgsteal}_direct separately.
> 
> Also, for forward compatibility, we still display pgscan and
> pgsteal items so that it won't break existing applications.

I do not remember why we have chosen to report cumulative stats rather
than the direct and kswapd parts. Looking back when Roman has introduced
those (http://lkml.kernel.org/r/1494530183-30808-1-git-send-email-guro@fb.com)
I do not see any discussion around that. So it was likely just not
a priority.

I have just one question. Say we even decide to have a per memcg kswapd
in some form, would we report that into the same counter?

> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Roman Gushchin <roman.gushchin@linux.dev>
> Acked-by: Muchun Song <songmuchun@bytedance.com>

In any case
Acked-by: Michal Hocko <mhocko@suse.com>

One nit below
[...]
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0d3fe0a0c75a..fd78c4d6bbc7 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1460,6 +1460,28 @@ static inline unsigned long memcg_page_state_output(struct mem_cgroup *memcg,
>  	return memcg_page_state(memcg, item) * memcg_page_state_unit(item);
>  }
>  

I would just add the following for clarity

/* Subset of vm_event_item to report for memcg event stats */
> +static const unsigned int memcg_vm_event_stat[] = {
> +	PGSCAN_KSWAPD,
> +	PGSCAN_DIRECT,
> +	PGSTEAL_KSWAPD,
> +	PGSTEAL_DIRECT,
> +	PGFAULT,
> +	PGMAJFAULT,
> +	PGREFILL,
> +	PGACTIVATE,
> +	PGDEACTIVATE,
> +	PGLAZYFREE,
> +	PGLAZYFREED,
> +#if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> +	ZSWPIN,
> +	ZSWPOUT,
> +#endif
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	THP_FAULT_ALLOC,
> +	THP_COLLAPSE_ALLOC,
> +#endif
> +};

-- 
Michal Hocko
SUSE Labs