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
