linux-kernel - Re: [RFC PATCH 0/3] mm: convert mm's rss stats into lazy_percpu

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]
Message-ID: <e30f3d21-6957-a5fd-d61f-44d7d52f63cb@huawei.com>
Date: Mon, 15 Apr 2024 20:33:44 +0800
From: "zhangpeng (AS)" <zhangpeng362@...wei.com>
To: Jan Kara <jack@...e.cz>
CC: <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
	<akpm@...ux-foundation.org>, <dennisszhou@...il.com>, <shakeelb@...gle.com>,
	<surenb@...gle.com>, <kent.overstreet@...ux.dev>, <mhocko@...e.cz>,
	<vbabka@...e.cz>, <yuzhao@...gle.com>, <yu.ma@...el.com>,
	<wangkefeng.wang@...wei.com>, <sunnanyong@...wei.com>
Subject: Re: [RFC PATCH 0/3] mm: convert mm's rss stats into
 lazy_percpu_counter

On 2024/4/12 21:53, Jan Kara wrote:

> On Fri 12-04-24 17:24:38, Peng Zhang wrote:
>> From: ZhangPeng <zhangpeng362@...wei.com>
>>
>> Since commit f1a7941243c1 ("mm: convert mm's rss stats into
>> percpu_counter"), the rss_stats have converted into percpu_counter,
>> which convert the error margin from (nr_threads * 64) to approximately
>> (nr_cpus ^ 2). However, the new percpu allocation in mm_init() causes a
>> performance regression on fork/exec/shell. Even after commit
>> 14ef95be6f55 ("kernel/fork: group allocation/free of per-cpu counters
>> for mm struct"), the performance of fork/exec/shell is still poor
>> compared to previous kernel versions.
>>
>> To mitigate performance regression, we use lazy_percpu_counter[1] to
>> delay the allocation of percpu memory for rss_stats. After lmbench test,
>> we will get 3% ~ 6% performance improvement for lmbench
>> fork_proc/exec_proc/shell_proc after conversion.
>>
>> The test results are as follows:
>>
>>               base           base+revert        base+lazy_percpu_counter
>>
>> fork_proc    427.4ms        394.1ms  (7.8%)    413.9ms  (3.2%)
>> exec_proc    2205.1ms       2042.2ms (7.4%)    2072.0ms (6.0%)
>> shell_proc   3180.9ms       2963.7ms (6.8%)    3010.7ms (5.4%)
>>
>> This solution has not been fully evaluated and tested. The main idea of
>> this RFC patch series is to get the community's opinion on this approach.
> Thanks! I like the idea and in fact I wanted to do something similar (just
> never got to it). Thread [2] has couple of good observations regarding this
> problem. Couple of thoughts regarding your approach:
>
> 1) I think switching to pcpu counter when update rate exceeds 256 updates/s
> is not a great fit for RSS because the updates are going to be frequent in
> some cases but usually they will all happen from one thread. So I think it
> would make more sense to move the decision of switching to pcpu mode from
> the counter itself into the callers and just switch on clone() when the
> second thread gets created.
>
> 2) I thought that for RSS lazy percpu counters, we could directly use
> struct percpu_counter and just make it that if 'counters' is NULL, the
> counter is in atomic mode (count is used as atomic_long_t), if counters !=
> NULL, we are in pcpu mode.

Thanks for your reply!
Agree with your thoughts, I'll implement it in the next version.

> 3) In [2] Mateusz had a good observation that the old RSS counters actually
> used atomic operations only in rare cases so even lazy pcpu counters are
> going to have worse performance for singlethreaded processes than the old
> code. We could *almost* get away with non-atomic updates to counter->count
> if it was not for occasional RSS updates from unrelated tasks. So it might
> be worth it to further optimize the counters as:
>
> struct rss_counter_single {
> 	void *state;			/* To detect switching to pcpu mode */
> 	atomic_long_t counter_atomic;	/* Used for foreign updates */
> 	long counter;			/* Used by local updates */
> }
>
> struct rss_counter {
> 	union {
> 		struct rss_counter_single single;
> 		/* struct percpu_counter needs to be modified to have
> 		 * 'counters' first to avoid issues for different
> 		 * architectures or with CONFIG_HOTPLUG_CPU enabled */
> 		struct percpu_counter pcpu;
> 	}
> }
>
> But I'm not sure this complexity is worth it so I'd do it as a separate
> patch with separate benchmarking if at all.
>
> 								Honza

Agreed. Single-threaded processes don't need atomic operations, and
this scenario needs to be thoroughly tested.
I'll try to implement it in another patch series after I finish the
basic approach.

> [2] https://lore.kernel.org/all/ZOPSEJTzrow8YFix@snowbird/
>
>> [1] https://lore.kernel.org/linux-iommu/20230501165450.15352-8-surenb@google.com/
>>
>> Kent Overstreet (1):
>>    Lazy percpu counters
>>
>> ZhangPeng (2):
>>    lazy_percpu_counter: include struct percpu_counter in struct
>>      lazy_percpu_counter
>>    mm: convert mm's rss stats into lazy_percpu_counter
>>
>>   include/linux/lazy-percpu-counter.h |  88 +++++++++++++++++++
>>   include/linux/mm.h                  |   8 +-
>>   include/linux/mm_types.h            |   4 +-
>>   include/trace/events/kmem.h         |   4 +-
>>   kernel/fork.c                       |  12 +--
>>   lib/Makefile                        |   2 +-
>>   lib/lazy-percpu-counter.c           | 131 ++++++++++++++++++++++++++++
>>   7 files changed, 232 insertions(+), 17 deletions(-)
>>   create mode 100644 include/linux/lazy-percpu-counter.h
>>   create mode 100644 lib/lazy-percpu-counter.c
>>
>> -- 
>> 2.25.1
>>
-- 
Best Regards,
Peng