Re: [RFC PATCH v2 2/2] mm: convert mm's rss stats to use atomic mode

[Rongwei Wang <rongwei.wrw@xxxxxxxxx>]

On 2024/4/18 22:20, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@xxxxxxxxxx>
>
> 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 delay the allocation of percpu
> memory for rss_stats. Therefore, we convert mm's rss stats to use
> percpu_counter atomic mode. For single-thread processes, rss_stat is in
> atomic mode, which reduces the memory consumption and performance
> regression caused by using percpu. For multiple-thread processes,
> rss_stat is switched to the percpu mode to reduce the error margin.
> We convert rss_stats from atomic mode to percpu mode only when the
> second thread is created.
Hi, Zhang Peng

This regression we also found it in lmbench these days. I have not test 
your patch, but it seems will solve a lot for it.
And I see this patch not fix the regression in multi-threads, that's 
because of the rss_stat switched to percpu mode?
(If I'm wrong, please correct me.) And It seems percpu_counter also has 
a bad effect in exit_mmap().

If so, I'm wondering if we can further improving it on the exit_mmap() 
path in multi-threads scenario, e.g. to determine which CPUs the process 
has run on (mm_cpumask()? I'm not sure).

> After lmbench test, we can get 2% ~ 4% performance improvement
> for lmbench fork_proc/exec_proc/shell_proc and 6.7% performance
> improvement for lmbench page_fault (before batch mode[1]).
>
> The test results are as follows:
>
>               base           base+revert        base+this patch
>
> fork_proc    416.3ms        400.0ms  (3.9%)    398.6ms  (4.2%)
> exec_proc    2095.9ms       2061.1ms (1.7%)    2047.7ms (2.3%)
> shell_proc   3028.2ms       2954.7ms (2.4%)    2961.2ms (2.2%)
> page_fault   0.3603ms       0.3358ms (6.8%)    0.3361ms (6.7%)
I think the regression will becomes more obvious if more cores. How 
about your test machine?

Thanks,
-wrw
> [1] https://lore.kernel.org/all/20240412064751.119015-1-wangkefeng.wang@xxxxxxxxxx/
>
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: ZhangPeng <zhangpeng362@xxxxxxxxxx>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
> ---
>   include/linux/mm.h          | 50 +++++++++++++++++++++++++++++++------
>   include/trace/events/kmem.h |  4 +--
>   kernel/fork.c               | 18 +++++++------
>   3 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index d261e45bb29b..8f1bfbd54697 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2631,30 +2631,66 @@ static inline bool get_user_page_fast_only(unsigned long addr,
>    */
>   static inline unsigned long get_mm_counter(struct mm_struct *mm, int member)
>   {
> -	return percpu_counter_read_positive(&mm->rss_stat[member]);
> +	struct percpu_counter *fbc = &mm->rss_stat[member];
> +
> +	if (percpu_counter_initialized(fbc))
> +		return percpu_counter_read_positive(fbc);
> +
> +	return percpu_counter_atomic_read(fbc);
>   }
>   
>   void mm_trace_rss_stat(struct mm_struct *mm, int member);
>   
>   static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
>   {
> -	percpu_counter_add(&mm->rss_stat[member], value);
> +	struct percpu_counter *fbc = &mm->rss_stat[member];
> +
> +	if (percpu_counter_initialized(fbc))
> +		percpu_counter_add(fbc, value);
> +	else
> +		percpu_counter_atomic_add(fbc, value);
>   
>   	mm_trace_rss_stat(mm, member);
>   }
>   
>   static inline void inc_mm_counter(struct mm_struct *mm, int member)
>   {
> -	percpu_counter_inc(&mm->rss_stat[member]);
> -
> -	mm_trace_rss_stat(mm, member);
> +	add_mm_counter(mm, member, 1);
>   }
>   
>   static inline void dec_mm_counter(struct mm_struct *mm, int member)
>   {
> -	percpu_counter_dec(&mm->rss_stat[member]);
> +	add_mm_counter(mm, member, -1);
> +}
>   
> -	mm_trace_rss_stat(mm, member);
> +static inline s64 mm_counter_sum(struct mm_struct *mm, int member)
> +{
> +	struct percpu_counter *fbc = &mm->rss_stat[member];
> +
> +	if (percpu_counter_initialized(fbc))
> +		return percpu_counter_sum(fbc);
> +
> +	return percpu_counter_atomic_read(fbc);
> +}
> +
> +static inline s64 mm_counter_sum_positive(struct mm_struct *mm, int member)
> +{
> +	struct percpu_counter *fbc = &mm->rss_stat[member];
> +
> +	if (percpu_counter_initialized(fbc))
> +		return percpu_counter_sum_positive(fbc);
> +
> +	return percpu_counter_atomic_read(fbc);
> +}
> +
> +static inline int mm_counter_switch_to_pcpu_many(struct mm_struct *mm)
> +{
> +	return percpu_counter_switch_to_pcpu_many(mm->rss_stat, NR_MM_COUNTERS);
> +}
> +
> +static inline void mm_counter_destroy_many(struct mm_struct *mm)
> +{
> +	percpu_counter_destroy_many(mm->rss_stat, NR_MM_COUNTERS);
>   }
>   
>   /* Optimized variant when folio is already known not to be anon */
> diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
> index 6e62cc64cd92..a4e40ae6a8c8 100644
> --- a/include/trace/events/kmem.h
> +++ b/include/trace/events/kmem.h
> @@ -399,8 +399,8 @@ TRACE_EVENT(rss_stat,
>   		__entry->mm_id = mm_ptr_to_hash(mm);
>   		__entry->curr = !!(current->mm == mm);
>   		__entry->member = member;
> -		__entry->size = (percpu_counter_sum_positive(&mm->rss_stat[member])
> -							    << PAGE_SHIFT);
> +		__entry->size = (mm_counter_sum_positive(mm, member)
> +							<< PAGE_SHIFT);
>   	),
>   
>   	TP_printk("mm_id=%u curr=%d type=%s size=%ldB",
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 99076dbe27d8..0214273798c5 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -823,7 +823,7 @@ static void check_mm(struct mm_struct *mm)
>   			 "Please make sure 'struct resident_page_types[]' is updated as well");
>   
>   	for (i = 0; i < NR_MM_COUNTERS; i++) {
> -		long x = percpu_counter_sum(&mm->rss_stat[i]);
> +		long x = mm_counter_sum(mm, i);
>   
>   		if (unlikely(x))
>   			pr_alert("BUG: Bad rss-counter state mm:%p type:%s val:%ld\n",
> @@ -1301,16 +1301,10 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
>   	if (mm_alloc_cid(mm))
>   		goto fail_cid;
>   
> -	if (percpu_counter_init_many(mm->rss_stat, 0, GFP_KERNEL_ACCOUNT,
> -				     NR_MM_COUNTERS))
> -		goto fail_pcpu;
> -
>   	mm->user_ns = get_user_ns(user_ns);
>   	lru_gen_init_mm(mm);
>   	return mm;
>   
> -fail_pcpu:
> -	mm_destroy_cid(mm);
>   fail_cid:
>   	destroy_context(mm);
>   fail_nocontext:
> @@ -1730,6 +1724,16 @@ static int copy_mm(unsigned long clone_flags, struct task_struct *tsk)
>   	if (!oldmm)
>   		return 0;
>   
> +	/*
> +	 * For single-thread processes, rss_stat is in atomic mode, which
> +	 * reduces the memory consumption and performance regression caused by
> +	 * using percpu. For multiple-thread processes, rss_stat is switched to
> +	 * the percpu mode to reduce the error margin.
> +	 */
> +	if (clone_flags & CLONE_THREAD)
> +		if (mm_counter_switch_to_pcpu_many(oldmm))
> +			return -ENOMEM;
> +
>   	if (clone_flags & CLONE_VM) {
>   		mmget(oldmm);
>   		mm = oldmm;