Re: [PATCH v2] mm: memcontrol: add {pgscan,pgsteal}_{kswapd,direct} items in memory.stat of cgroup v2

[Qi Zheng]

On 2022/6/6 8:03 PM, Michal Hocko wrote:
> 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@xxxxxx)
> 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?

IMO, I would like it can be reported into the same counter.

> > Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> > Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> > Acked-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> > Acked-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>
> In any case
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>

Thanks.

> 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

OK, will do.

> /* 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
> > +};

--
Thanks,
Qi