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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 6 Jun 2022 21:29:58 +0800
From:   Qi Zheng <zhengqi.arch@...edance.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     hannes@...xchg.org, roman.gushchin@...ux.dev, shakeelb@...gle.com,
        songmuchun@...edance.com, akpm@...ux-foundation.org,
        corbet@....net, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v2] mm: memcontrol: add {pgscan,pgsteal}_{kswapd,direct}
 items in memory.stat of cgroup v2



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@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?

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

> 
>> Signed-off-by: Qi Zheng <zhengqi.arch@...edance.com>
>> Acked-by: Johannes Weiner <hannes@...xchg.org>
>> Acked-by: Roman Gushchin <roman.gushchin@...ux.dev>
>> Acked-by: Muchun Song <songmuchun@...edance.com>
> 
> In any case
> Acked-by: Michal Hocko <mhocko@...e.com>

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