Re: [PATCH linux-mm v2] mm: make pcp_decay_high working better with NOHZ full

[Huang, Ying]

Hi, Mengen,

mengensun88@xxxxxxxxx writes:

> From: MengEn Sun <mengensun@xxxxxxxxxxx>
>
> When a cpu entring NOHZ full, quiet_vmstat may flush percpu
> zonestats and nodestats.
>
> The vmstat_shepherd only check percpu zonestats and nodestats
> to determine whether it is necessary to fire vmstat_update on
> the target cpu for now.
>
> If a process on a certain CPU allocates a large amount of memory,
> then frees that memory, and subsequently the CPU enters NOHZ, and
> the process not freeing and allocating memory anymore,the
> vmstat_update not being executed on the cpu. Because
> vmstat_shepherd may not see zonestats and nodestats of the cpu
> changed, so may resulting in vmstat_update on the cpu not fired
> for a long time.

The issue description is too long, can you make it a little shorter?
And can you correct your grammar with some tool?  Something like chatgpt
is good for that, e.g., "fix the grammar of the following text: ...".
To make variable and function name distinct, I personally prefer to add
"()" after the function name.

Have verified the issue with some test?  If not, I suggest you to do
that.

> While, This seems to be fine:
> - if freeing and allocating memory occur later, it may the
>   high_max may be adjust automatically
> - If memory is tight, the memory reclamation process will
>   release the pcp

This could be a real issue for me.

> Whatever, we make vmstat_shepherd to checking whether we need
> decay pcp high_max, and fire pcp_decay_high early if we need.
>
> Fixes: 51a755c56dc0 ("mm: tune PCP high automatically")
> Reviewed-by: Jinliang Zheng <alexjlzheng@xxxxxxxxxxx>
> Signed-off-by: MengEn Sun <mengensun@xxxxxxxxxxx>
> ---
> changelog:
> v1: https://lore.kernel.org/lkml/20241012154328.015f57635566485ad60712f3@xxxxxxxxxxxxxxxxxxxx/T/#t
> v2: Make the commit message clearer by adding some comments.
> ---
>  mm/vmstat.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 1917c034c045..07b494b06872 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -2024,8 +2024,17 @@ static bool need_update(int cpu)
>  
>  	for_each_populated_zone(zone) {
>  		struct per_cpu_zonestat *pzstats = per_cpu_ptr(zone->per_cpu_zonestats, cpu);
> +		struct per_cpu_pages *pcp = per_cpu_ptr(zone->per_cpu_pageset, cpu);
>  		struct per_cpu_nodestat *n;
>  
> +		/* per_cpu_nodestats and per_cpu_zonestats maybe flush when cpu
> +		 * entering NOHZ full, see quiet_vmstat. so, we check pcp
> +		 * high_{min,max} to determine whether it is necessary to run
> +		 * decay_pcp_high on the corresponding CPU
> +		 */

Please follow the comments coding style.

                /*
                 * comments line 1
                 * comments line 2
                 */

> +		if (pcp->high_max > pcp->high_min)
> +			return true;
> +

We don't tune pcp->high_max/min in fact.  Instead, we tune pcp->high.
Your code may make need_update() return true in most cases.

>  		/*
>  		 * The fast way of checking if there are any vmstat diffs.
>  		 */

--
Best Regards,
Huang, Ying