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

From: Huang, Ying
Date: Mon Oct 21 2024 - 04:44:13 EST


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