Re: [PATCH v3] vmstat: disable vmstat_work on vmstat_cpu_down_prep()

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 1/8/25 09:58, Koichiro Den wrote:
> The upstream commit adcfb264c3ed ("vmstat: disable vmstat_work on
> vmstat_cpu_down_prep()") introduced another warning during the boot phase

I did observe this warning during a boot on arm64 and a quick git bisect also
pointed to the above mentioned commit. But seems like you are already on this
problem.

[    0.092532] ------------[ cut here ]------------                                                  
[    0.092534] workqueue: work disable count underflowed                                             
[    0.092540] WARNING: CPU: 1 PID: 21 at kernel/workqueue.c:4313 enable_work+0xe8/0x100             
[    0.092550] Modules linked in:                                                                    
[    0.092554] CPU: 1 UID: 0 PID: 21 Comm: cpuhp/1 Not tainted 6.13.0-rc4-00018-gadcfb264c3ed #11    
[    0.092558] Hardware name: linux,dummy-virt (DT)                                                  
[    0.092560] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)                       
[    0.092564] pc : enable_work+0xe8/0x100                                                           
[    0.092566] lr : enable_work+0xe8/0x100                                                           
[    0.092569] sp : ffff800082de3d50                                                                 
[    0.092570] x29: ffff800082de3d50 x28: 0000000000000000 x27: 0000000000000000                     
[    0.092574] x26: 0000000000000000 x25: 0000000000000000 x24: ffff00008001a1d0                     
[    0.092578] x23: 0000000000000001 x22: 0000000000000001 x21: ffff0001ff2de110                     
[    0.092582] x20: ffff8000827506a0 x19: ffff0001ff2e4d28 x18: 0000000000000038
[    0.092585] x17: 000000040044ffff x16: 00500074b5503510 x15: fffffffffffe1698
[    0.092589] x14: ffff8000827547d8 x13: 00000000000000ff x12: 0000000000000055
[    0.092592] x11: fffffffffffe1698 x10: ffff8000827ac7d8 x9 : 00000000fffff000
[    0.092596] x8 : ffff8000827547d8 x7 : ffff8000827ac7d8 x6 : 0000000000000000
[    0.092599] x5 : 80000000fffff000 x4 : 000000000000aff5 x3 : 0000000000000000
[    0.092603] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000080270000
[    0.092607] Call trace:
[    0.092608]  enable_work+0xe8/0x100 (P)
[    0.092612]  enable_delayed_work+0x10/0x1c
[    0.092615]  vmstat_cpu_online+0x84/0xb8
[    0.092621]  cpuhp_invoke_callback+0x104/0x20c
[    0.092626]  cpuhp_thread_fun+0xa4/0x17c
[    0.092629]  smpboot_thread_fn+0x220/0x24c
[    0.092635]  kthread+0x118/0x11c
[    0.092640]  ret_from_fork+0x10/0x20
[    0.092645] ---[ end trace 0000000000000000 ]---

> so was soon reverted on upstream by commit cd6313beaeae ("Revert "vmstat:
> disable vmstat_work on vmstat_cpu_down_prep()""). This commit resolves it
> and reattempts the original fix.
> 
> Even after mm/vmstat:online teardown, shepherd may still queue work for
> the dying cpu until the cpu is removed from online mask. While it's quite
> rare, this means that after unbind_workers() unbinds a per-cpu kworker, it
> potentially runs vmstat_update for the dying CPU on an irrelevant cpu
> before entering atomic AP states. When CONFIG_DEBUG_PREEMPT=y, it results
> in the following error with the backtrace.
> 
>   BUG: using smp_processor_id() in preemptible [00000000] code: \
>                                                kworker/7:3/1702
>   caller is refresh_cpu_vm_stats+0x235/0x5f0
>   CPU: 0 UID: 0 PID: 1702 Comm: kworker/7:3 Tainted: G
>   Tainted: [N]=TEST
>   Workqueue: mm_percpu_wq vmstat_update
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x8d/0xb0
>    check_preemption_disabled+0xce/0xe0
>    refresh_cpu_vm_stats+0x235/0x5f0
>    vmstat_update+0x17/0xa0
>    process_one_work+0x869/0x1aa0
>    worker_thread+0x5e5/0x1100
>    kthread+0x29e/0x380
>    ret_from_fork+0x2d/0x70
>    ret_from_fork_asm+0x1a/0x30
>    </TASK>
> 
> So, for mm/vmstat:online, disable vmstat_work reliably on teardown and
> symmetrically enable it on startup.
> 
> For secondary CPUs during CPU hotplug scenarios, ensure the delayed work
> is disabled immediately after the initialization. These CPUs are not yet
> online when start_shepherd_timer() runs on boot CPU. vmstat_cpu_online()
> will enable the work for them.
> 
> Suggested-by: Huacai Chen <chenhuacai@kernel.org>
> Signed-off-by: Huacai Chen <chenhuacai@kernel.org>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
> v2: https://lore.kernel.org/all/20241221033321.4154409-1-koichiro.den@canonical.com/
> v1: https://lore.kernel.org/all/20241220134234.3809621-1-koichiro.den@canonical.com/
> ---
>  mm/vmstat.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 4d016314a56c..16bfe1c694dd 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -2122,10 +2122,20 @@ static void __init start_shepherd_timer(void)
>  {
>  	int cpu;
>  
> -	for_each_possible_cpu(cpu)
> +	for_each_possible_cpu(cpu) {
>  		INIT_DEFERRABLE_WORK(per_cpu_ptr(&vmstat_work, cpu),
>  			vmstat_update);
>  
> +		/*
> +		 * For secondary CPUs during CPU hotplug scenarios,
> +		 * vmstat_cpu_online() will enable the work.
> +		 * mm/vmstat:online enables and disables vmstat_work
> +		 * symmetrically during CPU hotplug events.
> +		 */
> +		if (!cpu_online(cpu))
> +			disable_delayed_work_sync(&per_cpu(vmstat_work, cpu));
> +	}
> +
>  	schedule_delayed_work(&shepherd,
>  		round_jiffies_relative(sysctl_stat_interval));
>  }
> @@ -2148,13 +2158,14 @@ static int vmstat_cpu_online(unsigned int cpu)
>  	if (!node_state(cpu_to_node(cpu), N_CPU)) {
>  		node_set_state(cpu_to_node(cpu), N_CPU);
>  	}
> +	enable_delayed_work(&per_cpu(vmstat_work, cpu));
>  
>  	return 0;
>  }
>  
>  static int vmstat_cpu_down_prep(unsigned int cpu)
>  {
> -	cancel_delayed_work_sync(&per_cpu(vmstat_work, cpu));
> +	disable_delayed_work_sync(&per_cpu(vmstat_work, cpu));
>  	return 0;
>  }
>