Re: [PATCH v3 3/3] sched/cpuacct: remove redundant RCU read lock

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 20.02.2022 06:14, Chengming Zhou wrote:
> The cpuacct_account_field() and it's cgroup v2 wrapper
> cgroup_account_cputime_field() is only called from cputime
> in task_group_account_field(), which is already in RCU read-side
> critical section. So remove these redundant RCU read lock.
>
> Suggested-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>

This patch landed recently in linux-next as commit 3eba0505d03a 
("sched/cpuacct: Remove redundant RCU read lock"). On my test systems I 
found that it triggers a following warning in the early boot stage:

Calibrating delay loop (skipped), value calculated using timer 
frequency.. 48.00 BogoMIPS (lpj=240000)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
Mountpoint-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
CPU: Testing write buffer coherency: ok
CPU0: Spectre v2: using BPIALL workaround
CPU0: thread -1, cpu 0, socket 9, mpidr 80000900
cblist_init_generic: Setting adjustable number of callback queues.

=============================
WARNING: suspicious RCU usage
5.17.0-rc5-00052-g167ee9b08bed #11459 Not tainted
-----------------------------
./include/linux/cgroup.h:481 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 1, debug_locks = 1
no locks held by swapper/0/1.

stack backtrace:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.17.0-rc5-00052-g167ee9b08bed 
#11459
Hardware name: Samsung Exynos (Flattened Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x58/0x70
  dump_stack_lvl from account_system_index_time+0x13c/0x148
  account_system_index_time from account_system_time+0x78/0xa0
  account_system_time from update_process_times+0x50/0xc4
  update_process_times from tick_handle_periodic+0x1c/0x94
  tick_handle_periodic from exynos4_mct_tick_isr+0x30/0x38
  exynos4_mct_tick_isr from __handle_irq_event_percpu+0x74/0x3ac
  __handle_irq_event_percpu from handle_irq_event_percpu+0xc/0x38
  handle_irq_event_percpu from handle_irq_event+0x38/0x5c
  handle_irq_event from handle_fasteoi_irq+0xc4/0x180
  handle_fasteoi_irq from generic_handle_domain_irq+0x44/0x88
  generic_handle_domain_irq from gic_handle_irq+0x88/0xac
  gic_handle_irq from generic_handle_arch_irq+0x58/0x78
  generic_handle_arch_irq from __irq_svc+0x54/0x88
Exception stack(0xc1cafea0 to 0xc1cafee8)
fea0: 00000000 c0f33ac4 00000001 00000129 60000053 c127b458 00000008 
2e64e000
fec0: ef7bd5fc c127af84 c1209048 c1208f10 00000000 c1cafef0 c0b7e25c 
c0b8976c
fee0: 20000053 ffffffff
  __irq_svc from _raw_spin_unlock_irqrestore+0x2c/0x60
  _raw_spin_unlock_irqrestore from 
cblist_init_generic.constprop.14+0x298/0x328
  cblist_init_generic.constprop.14 from rcu_init_tasks_generic+0x18/0x110
  rcu_init_tasks_generic from kernel_init_freeable+0xa0/0x214
  kernel_init_freeable from kernel_init+0x18/0x12c
  kernel_init from ret_from_fork+0x14/0x2c
Exception stack(0xc1caffb0 to 0xc1cafff8)
ffa0:                                     00000000 00000000 00000000 
00000000
ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
cblist_init_generic: Setting shift to 1 and lim to 1.
Running RCU-tasks wait API self tests
Setting up static identity map for 0x40100000 - 0x40100060
rcu: Hierarchical SRCU implementation.
smp: Bringing up secondary CPUs ...
CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
CPU1: Spectre v2: using BPIALL workaround
smp: Brought up 1 node, 2 CPUs
SMP: Total of 2 processors activated (96.00 BogoMIPS).
CPU: All CPU(s) started in SVC mode.

The above log comes from ARM 32bit Samsung Exnyos4210 based Trats board. 
To get above log I reverted commit dc6e0818bc9a ("sched/cpuacct: 
Optimize away RCU read lock"), because it triggered a similar warning.

> ---
>   include/linux/cgroup.h | 2 --
>   kernel/sched/cpuacct.c | 2 --
>   2 files changed, 4 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 9a109c6ac0e0..1e356c222756 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -804,11 +804,9 @@ static inline void cgroup_account_cputime_field(struct task_struct *task,
>   
>   	cpuacct_account_field(task, index, delta_exec);
>   
> -	rcu_read_lock();
>   	cgrp = task_dfl_cgroup(task);
>   	if (cgroup_parent(cgrp))
>   		__cgroup_account_cputime_field(cgrp, index, delta_exec);
> -	rcu_read_unlock();
>   }
>   
>   #else	/* CONFIG_CGROUPS */
> diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
> index f79f88456d72..d269ede84e85 100644
> --- a/kernel/sched/cpuacct.c
> +++ b/kernel/sched/cpuacct.c
> @@ -352,10 +352,8 @@ void cpuacct_account_field(struct task_struct *tsk, int index, u64 val)
>   {
>   	struct cpuacct *ca;
>   
> -	rcu_read_lock();
>   	for (ca = task_ca(tsk); ca != &root_cpuacct; ca = parent_ca(ca))
>   		__this_cpu_add(ca->cpustat->cpustat[index], val);
> -	rcu_read_unlock();
>   }
>   
>   struct cgroup_subsys cpuacct_cgrp_subsys = {

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland