PM / OPP: cpufreq: Avoid sleeping while atomic
Subsystems (2)
PATCH 1/1 PM / OPP: cpufreq: Avoid sleeping while atomic
We allocate the cpufreq table after calling rcu_read_lock(), which disables preemption. This causes scheduling while atomic warnings. Use GFP_ATOMIC instead of GFP_KERNEL and update for kcalloc while we're here.
BUG: sleeping function called from invalid context at mm/slub.c:1246
in_atomic(): 0, irqs_disabled(): 0, pid: 80, name: modprobe
5 locks held by modprobe/80:
#0: (&dev->mutex){......}, at: [<c050d484>] __driver_attach+0x48/0x98
#1: (&dev->mutex){......}, at: [<c050d494>] __driver_attach+0x58/0x98
#2: (subsys mutex#5){+.+.+.}, at: [<c050c114>] subsys_interface_register+0x38/0xc8
#3: (cpufreq_rwsem){.+.+.+}, at: [<c05a9c8c>] __cpufreq_add_dev.isra.22+0x84/0x92c
#4: (rcu_read_lock){......}, at: [<c05ab24c>] dev_pm_opp_init_cpufreq_table+0x18/0x10cPreemption disabled at:[< (null)>] (null)
CPU: 2 PID: 80 Comm: modprobe Not tainted 3.16.0-rc3-next-20140701-00035-g286857f216aa-dirty #217 [<c0214da8>] (unwind_backtrace) from [<c02123f8>] (show_stack+0x10/0x14) [<c02123f8>] (show_stack) from [<c070141c>] (dump_stack+0x70/0xbc) [<c070141c>] (dump_stack) from [<c02f4cb0>] (__kmalloc+0x124/0x250) [<c02f4cb0>] (__kmalloc) from [<c05ab270>] (dev_pm_opp_init_cpufreq_table+0x3c/0x10c) [<c05ab270>] (dev_pm_opp_init_cpufreq_table) from [<bf000508>] (cpufreq_init+0x48/0x378 [cpufreq_generic]) [<bf000508>] (cpufreq_init [cpufreq_generic]) from [<c05a9e08>] (__cpufreq_add_dev.isra.22+0x200/0x92c) [<c05a9e08>] (__cpufreq_add_dev.isra.22) from [<c050c160>] (subsys_interface_register+0x84/0xc8) [<c050c160>] (subsys_interface_register) from [<c05a9494>] (cpufreq_register_driver+0x108/0x2d8) [<c05a9494>] (cpufreq_register_driver) from [<bf000888>] (generic_cpufreq_probe+0x50/0x74 [cpufreq_generic]) [<bf000888>] (generic_cpufreq_probe [cpufreq_generic]) from [<c050e994>] (platform_drv_probe+0x18/0x48) [<c050e994>] (platform_drv_probe) from [<c050d1f4>] (driver_probe_device+0x128/0x370) [<c050d1f4>] (driver_probe_device) from [<c050d4d0>] (__driver_attach+0x94/0x98) [<c050d4d0>] (__driver_attach) from [<c050b778>] (bus_for_each_dev+0x54/0x88) [<c050b778>] (bus_for_each_dev) from [<c050c894>] (bus_add_driver+0xe8/0x204) [<c050c894>] (bus_add_driver) from [<c050dd48>] (driver_register+0x78/0xf4) [<c050dd48>] (driver_register) from [<c0208870>] (do_one_initcall+0xac/0x1d8) [<c0208870>] (do_one_initcall) from [<c028b6b4>] (load_module+0x190c/0x21e8) [<c028b6b4>] (load_module) from [<c028c034>] (SyS_init_module+0xa4/0x110) [<c028c034>] (SyS_init_module) from [<c020f0c0>] (ret_fast_syscall+0x0/0x48)
---
It would be nice to not do atomic allocations, but I guess the table could change size?
drivers/cpufreq/cpufreq_opp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/cpufreq_opp.c +1 -1
@@ -60,7 +60,7 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, goto out; } - freq_table =kzalloc (sizeof (*freq_table) * (max_opps +1 ),GFP_KERNEL ); + freq_table =kcalloc (sizeof (*freq_table), (max_opps +1 ),GFP_ATOMIC );if (!freq_table) { ret = -ENOMEM ; goto out;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 4 lines · Stephen Boyd
We allocate the cpufreq table after calling
rcu_read_lock(), which disables preemption. This causes scheduling while atomic warnings. UseGFP_ATOMICinstead ofGFP_KERNELand update for kcalloc while we're here.
I am surprised to see that this isn't reported by anybody since the time it came into existence? Some special config option required to observe this?
Quoted 31 lines
BUG: sleeping function called from invalid context at mm/slub.c:1246 in_atomic(): 0, irqs_disabled(): 0, pid: 80, name: modprobe 5 locks held by modprobe/80: #0: (&dev->mutex){......}, at: [<c050d484>] __driver_attach+0x48/0x98 #1: (&dev->mutex){......}, at: [<c050d494>] __driver_attach+0x58/0x98 #2: (subsys mutex#5){+.+.+.}, at: [<c050c114>] subsys_interface_register+0x38/0xc8 #3: (cpufreq_rwsem){.+.+.+}, at: [<c05a9c8c>] __cpufreq_add_dev.isra.22+0x84/0x92c #4: (rcu_read_lock){......}, at: [<c05ab24c>] dev_pm_opp_init_cpufreq_table+0x18/0x10cPreemption disabled at:[< (null)>] (null)
CPU: 2 PID: 80 Comm: modprobe Not tainted 3.16.0-rc3-next-20140701-00035-g286857f216aa-dirty #217 [<c0214da8>] (unwind_backtrace) from [<c02123f8>] (show_stack+0x10/0x14) [<c02123f8>] (show_stack) from [<c070141c>] (dump_stack+0x70/0xbc) [<c070141c>] (dump_stack) from [<c02f4cb0>] (__kmalloc+0x124/0x250) [<c02f4cb0>] (__kmalloc) from [<c05ab270>] (dev_pm_opp_init_cpufreq_table+0x3c/0x10c) [<c05ab270>] (dev_pm_opp_init_cpufreq_table) from [<bf000508>] (cpufreq_init+0x48/0x378 [cpufreq_generic]) [<bf000508>] (cpufreq_init [cpufreq_generic]) from [<c05a9e08>] (__cpufreq_add_dev.isra.22+0x200/0x92c) [<c05a9e08>] (__cpufreq_add_dev.isra.22) from [<c050c160>] (subsys_interface_register+0x84/0xc8) [<c050c160>] (subsys_interface_register) from [<c05a9494>] (cpufreq_register_driver+0x108/0x2d8) [<c05a9494>] (cpufreq_register_driver) from [<bf000888>] (generic_cpufreq_probe+0x50/0x74 [cpufreq_generic]) [<bf000888>] (generic_cpufreq_probe [cpufreq_generic]) from [<c050e994>] (platform_drv_probe+0x18/0x48) [<c050e994>] (platform_drv_probe) from [<c050d1f4>] (driver_probe_device+0x128/0x370) [<c050d1f4>] (driver_probe_device) from [<c050d4d0>] (__driver_attach+0x94/0x98) [<c050d4d0>] (__driver_attach) from [<c050b778>] (bus_for_each_dev+0x54/0x88) [<c050b778>] (bus_for_each_dev) from [<c050c894>] (bus_add_driver+0xe8/0x204) [<c050c894>] (bus_add_driver) from [<c050dd48>] (driver_register+0x78/0xf4) [<c050dd48>] (driver_register) from [<c0208870>] (do_one_initcall+0xac/0x1d8) [<c0208870>] (do_one_initcall) from [<c028b6b4>] (load_module+0x190c/0x21e8) [<c028b6b4>] (load_module) from [<c028c034>] (SyS_init_module+0xa4/0x110) [<c028c034>] (SyS_init_module) from [<c020f0c0>] (ret_fast_syscall+0x0/0x48)
Quoted 1 line
Fixes a0dd7b79657b "PM / OPP: Move cpufreq specific OPP functions out of generic OPP library"
That looks to be wrong. This commit just moved things around and I can still see rcu_read_lock() before this commit.
Quoted 7 lines
Cc Kevin Hilman <[email protected]>Cc Nishanth Menon <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
It would be nice to not do atomic allocations, but I guess the table could change size?
Yep. Number of OPPs can vary over time on a running machine.
drivers/cpufreq/cpufreq_opp.c int dev_pm_opp_init_cpufreq_table(struct device *dev, · line 63
Quoted 13 lines
drivers/cpufreq/cpufreq_opp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/cpufreq/cpufreq_opp.c +1
-1@@ -60,7 +60,7 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, goto out; } - freq_table =kzalloc (sizeof (*freq_table) * (max_opps +1 ),GFP_KERNEL ); + freq_table =kcalloc (sizeof (*freq_table), (max_opps +1 ),GFP_ATOMIC );
I am not really sure if there would be any consequences of this, but overall it looks fine.
Quoted 8 lines · Viresh Kumar
Quoted 4 lines · Stephen Boyd
We allocate the cpufreq table after calling
rcu_read_lock(), which disables preemption. This causes scheduling while atomic warnings. UseGFP_ATOMICinstead ofGFP_KERNELand update for kcalloc while we're here.I am surprised to see that this isn't reported by anybody since the time it came into existence? Some special config option required to observe this?
First you need to enable sleeping while atomic checking, but in reality, I assume nobody has tried inserting a cpufreq driver as a module. The might_sleep() code has a check to see if the system_state is SYSTEM_RUNNING. If it isn't running then there isn't a warning and might_sleep() doesn't flag any problem. I wonder if that is actually the right thing to do though? Perhaps the intention of that code is to skip warning early on in the boot path when the scheduler isn't up and running yet. But once the scheduler is running (which is fairly early nowadays) I would think we want might_sleep() to trigger warnings. Maybe that check in might_sleep() needs to be updated to check for "scheduler running" instead of "system running"?
Quoted 32 lines
Quoted 32 lines
BUG: sleeping function called from invalid context at mm/slub.c:1246 in_atomic(): 0, irqs_disabled(): 0, pid: 80, name: modprobe 5 locks held by modprobe/80: #0: (&dev->mutex){......}, at: [<c050d484>] __driver_attach+0x48/0x98 #1: (&dev->mutex){......}, at: [<c050d494>] __driver_attach+0x58/0x98 #2: (subsys mutex#5){+.+.+.}, at: [<c050c114>] subsys_interface_register+0x38/0xc8 #3: (cpufreq_rwsem){.+.+.+}, at: [<c05a9c8c>] __cpufreq_add_dev.isra.22+0x84/0x92c #4: (rcu_read_lock){......}, at: [<c05ab24c>] dev_pm_opp_init_cpufreq_table+0x18/0x10cPreemption disabled at:[< (null)>] (null)
CPU: 2 PID: 80 Comm: modprobe Not tainted 3.16.0-rc3-next-20140701-00035-g286857f216aa-dirty #217 [<c0214da8>] (unwind_backtrace) from [<c02123f8>] (show_stack+0x10/0x14) [<c02123f8>] (show_stack) from [<c070141c>] (dump_stack+0x70/0xbc) [<c070141c>] (dump_stack) from [<c02f4cb0>] (__kmalloc+0x124/0x250) [<c02f4cb0>] (__kmalloc) from [<c05ab270>] (dev_pm_opp_init_cpufreq_table+0x3c/0x10c) [<c05ab270>] (dev_pm_opp_init_cpufreq_table) from [<bf000508>] (cpufreq_init+0x48/0x378 [cpufreq_generic]) [<bf000508>] (cpufreq_init [cpufreq_generic]) from [<c05a9e08>] (__cpufreq_add_dev.isra.22+0x200/0x92c) [<c05a9e08>] (__cpufreq_add_dev.isra.22) from [<c050c160>] (subsys_interface_register+0x84/0xc8) [<c050c160>] (subsys_interface_register) from [<c05a9494>] (cpufreq_register_driver+0x108/0x2d8) [<c05a9494>] (cpufreq_register_driver) from [<bf000888>] (generic_cpufreq_probe+0x50/0x74 [cpufreq_generic]) [<bf000888>] (generic_cpufreq_probe [cpufreq_generic]) from [<c050e994>] (platform_drv_probe+0x18/0x48) [<c050e994>] (platform_drv_probe) from [<c050d1f4>] (driver_probe_device+0x128/0x370) [<c050d1f4>] (driver_probe_device) from [<c050d4d0>] (__driver_attach+0x94/0x98) [<c050d4d0>] (__driver_attach) from [<c050b778>] (bus_for_each_dev+0x54/0x88) [<c050b778>] (bus_for_each_dev) from [<c050c894>] (bus_add_driver+0xe8/0x204) [<c050c894>] (bus_add_driver) from [<c050dd48>] (driver_register+0x78/0xf4) [<c050dd48>] (driver_register) from [<c0208870>] (do_one_initcall+0xac/0x1d8) [<c0208870>] (do_one_initcall) from [<c028b6b4>] (load_module+0x190c/0x21e8) [<c028b6b4>] (load_module) from [<c028c034>] (SyS_init_module+0xa4/0x110) [<c028c034>] (SyS_init_module) from [<c020f0c0>] (ret_fast_syscall+0x0/0x48)
Quoted 4 lines
Quoted 1 line
Fixes a0dd7b79657b "PM / OPP: Move cpufreq specific OPP functions out of generic OPP library"That looks to be wrong. This commit just moved things around and I can still see
rcu_read_lock()before this commit.
Right. It seems that we moved to RCU in commit 0f5c890e9b9754d9aa5bf6ae2fc00cae65780d23 so the real Fixes line should be:
internal data organization"
One way to avoid this problem is to put things back the way they were before that change. Is there any real benefit to having this code live in drivers/cpufreq/ instead of just under some config option in drivers/base/power/opp.c?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerQuoted 2 lines · Stephen Boyd
First you need to enable sleeping while atomic checking, but in reality, I assume nobody has tried inserting a cpufreq driver as a module. The
I did for sure, but long back. Over 6 months atleast :)
Quoted 12 lines
might_sleep()code has a check to see if thesystem_stateisSYSTEM_RUNNING. If it isn't running then there isn't a warning andmight_sleep()doesn't flag any problem. I wonder if that is actually the right thing to do though? Perhaps the intention of that code is to skip warning early on in the boot path when the scheduler isn't up and running yet. But once the scheduler is running (which is fairly early nowadays) I would think we wantmight_sleep()to trigger warnings. Maybe that check inmight_sleep()needs to be updated to check for "scheduler running" instead of "system running"?
Right. It seems that we moved to RCU in commit0f5c890e9b9754d9aa5bf6ae2fc00cae65780d23so the real Fixes line should be:
Quoted 2 lines
Fixes 0f5c890e9b97 "PM / OPP: Remove cpufreq wrapper dependency oninternal data organization"
Right.
Quoted 4 lines
One way to avoid this problem is to put things back the way they were before that change. Is there any real benefit to having this code live in
drivers/cpufreq/instead of just under some config option indrivers/base/power/opp.c?
Maybe Nishanth can give more arguments than I can :), but the idea was just to keep cpufreq stuff together..