Received-SPF: pass (google.com: domain of linux-kernel+bounces-142772-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 12 Apr 2024 15:53:33 +0200
From: Jan Kara <jack@suse.cz>
To: Peng Zhang <zhangpeng362@huawei.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	akpm@linux-foundation.org, dennisszhou@gmail.com,
	shakeelb@google.com, jack@suse.cz, surenb@google.com,
	kent.overstreet@linux.dev, mhocko@suse.cz, vbabka@suse.cz,
	yuzhao@google.com, yu.ma@intel.com, wangkefeng.wang@huawei.com,
	sunnanyong@huawei.com
Subject: Re: [RFC PATCH 0/3] mm: convert mm's rss stats into
 lazy_percpu_counter
Message-ID: <20240412135333.btd6e7wfprg4cmx2@quack3>
References: <20240412092441.3112481-1-zhangpeng362@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240412092441.3112481-1-zhangpeng362@huawei.com>

On Fri 12-04-24 17:24:38, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.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.

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

[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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR