| Message ID | 20220511093554.17535-1-cw00.choi@samsung.com |
|---|---|
| Headers | show |
| Series | PM / devfreq: Add cpu based scaling support to passive governor | expand |
Hi Chanwoo, On 11.05.2022 11:35, Chanwoo Choi wrote: > From: Saravana Kannan <skannan@codeaurora.org> > > Many CPU architectures have caches that can scale independent of the > CPUs. Frequency scaling of the caches is necessary to make sure that the > cache is not a performance bottleneck that leads to poor performance and > power. The same idea applies for RAM/DDR. > > To achieve this, this patch adds support for cpu based scaling to the > passive governor. This is accomplished by taking the current frequency > of each CPU frequency domain and then adjust the frequency of the cache > (or any devfreq device) based on the frequency of the CPUs. It listens > to CPU frequency transition notifiers to keep itself up to date on the > current CPU frequency. > > To decide the frequency of the device, the governor does one of the > following: > * Derives the optimal devfreq device opp from required-opps property of > the parent cpu opp_table. > > * Scales the device frequency in proportion to the CPU frequency. So, if > the CPUs are running at their max frequency, the device runs at its > max frequency. If the CPUs are running at their min frequency, the > device runs at its min frequency. It is interpolated for frequencies > in between. > > Tested-by: Chen-Yu Tsai <wenst@chromium.org> > Tested-by: Johnson Wang <johnson.wang@mediatek.com> > Signed-off-by: Saravana Kannan <skannan@codeaurora.org> > [Sibi: Integrated cpu-freqmap governor into passive_governor] > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > [Chanwoo: Fix conflict with latest code and cleanup code] > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> This patch landed in today's linux next-20220512 as commit 2ab415d4e4e6 ("PM / devfreq: Add cpu based scaling support to passive governor"). It triggers the following NULL pointer dereference on Exynos based boards: exynos-bus: new bus device registered: soc:bus-leftbus (100000 KHz ~ 200000 KHz) exynos-bus: new bus device registered: soc:bus-rightbus (100000 KHz ~ 200000 KHz) exynos-bus: new bus device registered: soc:bus-display (160000 KHz ~ 200000 KHz) exynos-bus: new bus device registered: soc:bus-fsys (100000 KHz ~ 134000 KHz) exynos-bus: new bus device registered: soc:bus-peri ( 50000 KHz ~ 100000 KHz) exynos-bus: new bus device registered: soc:bus-mfc (100000 KHz ~ 200000 KHz) 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 0000003c [0000003c] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 5.18.0-rc6-next-20220512 #5014 Hardware name: Samsung Exynos (Flattened Device Tree) Workqueue: devfreq_wq devfreq_monitor PC is at __mutex_lock+0x48/0x948 LR is at lock_is_held_type+0x104/0x1a4 pc : [<c0b93098>] lr : [<c0b8f2ec>] psr: 60000053 sp : f0889dc0 ip : 600000d3 fp : c1dca624 r10: c1dca44c r9 : 00000000 r8 : c1984104 r7 : c1d7f000 r6 : 00000000 r5 : 00000001 r4 : 00000008 r3 : 00000000 r2 : 00000000 r1 : 2de44000 r0 : 00000000 Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 4000404a DAC: 00000051 Register r0 information: NULL pointer Register r1 information: non-paged memory Register r2 information: NULL pointer Register r3 information: NULL pointer Register r4 information: non-paged memory Register r5 information: non-paged memory Register r6 information: NULL pointer Register r7 information: slab task_struct start c1d7f000 pointer offset 0 Register r8 information: non-slab/vmalloc memory Register r9 information: NULL pointer Register r10 information: slab kmalloc-2k start c1dca000 pointer offset 1100 size 2048 Register r11 information: slab kmalloc-2k start c1dca000 pointer offset 1572 size 2048 Register r12 information: non-paged memory Process kworker/u8:0 (pid: 8, stack limit = 0x(ptrval)) Stack: (0xf0889dc0 to 0xf088a000) ... __mutex_lock from mutex_lock_nested+0x1c/0x24 mutex_lock_nested from devfreq_passive_notifier_call+0x24/0x90 devfreq_passive_notifier_call from srcu_notifier_call_chain+0x98/0x114 srcu_notifier_call_chain from devfreq_set_target+0x6c/0x304 devfreq_set_target from devfreq_update_target+0x98/0xe8 devfreq_update_target from devfreq_monitor+0x28/0x1c0 devfreq_monitor from process_one_work+0x288/0x774 process_one_work from worker_thread+0x44/0x504 worker_thread from kthread+0xf4/0x128 kthread from ret_from_fork+0x14/0x2c Exception stack(0xf0889fb0 to 0xf0889ff8) ... ---[ end trace 0000000000000000 ]--- The issue is caused by the lack of setting devfreq_passive_data->this pointer in devfreq_passive_register_notifier. However, after adding: @@ -395,6 +395,9 @@ static int devfreq_passive_register_notifier(struct devfreq *devfreq) if (!parent) return -EPROBE_DEFER; + if (!p_data->this) + p_data->this = devfreq; + nb->notifier_call = devfreq_passive_notifier_call; return devfreq_register_notifier(parent, nb, DEVFREQ_TRANSITION_NOTIFIER); } the NULL pointer dereference is gone, but I see the following warnings on Odroid U3 board, which were not present before this patch: devfreq soc:bus-acp: failed to update devfreq using passive governor devfreq soc:bus-c2c: failed to update devfreq using passive governor devfreq soc:bus-acp: failed to update devfreq using passive governor devfreq soc:bus-c2c: failed to update devfreq using passive governor > --- > drivers/devfreq/governor.h | 22 +++ > drivers/devfreq/governor_passive.c | 297 +++++++++++++++++++++++++++-- > include/linux/devfreq.h | 17 +- > 3 files changed, 322 insertions(+), 14 deletions(-) > ... Best regards
On 22. 5. 13. 07:34, Marek Szyprowski wrote: > Hi Chanwoo, > > On 11.05.2022 11:35, Chanwoo Choi wrote: >> From: Saravana Kannan <skannan@codeaurora.org> >> >> Many CPU architectures have caches that can scale independent of the >> CPUs. Frequency scaling of the caches is necessary to make sure that the >> cache is not a performance bottleneck that leads to poor performance and >> power. The same idea applies for RAM/DDR. >> >> To achieve this, this patch adds support for cpu based scaling to the >> passive governor. This is accomplished by taking the current frequency >> of each CPU frequency domain and then adjust the frequency of the cache >> (or any devfreq device) based on the frequency of the CPUs. It listens >> to CPU frequency transition notifiers to keep itself up to date on the >> current CPU frequency. >> >> To decide the frequency of the device, the governor does one of the >> following: >> * Derives the optimal devfreq device opp from required-opps property of >> the parent cpu opp_table. >> >> * Scales the device frequency in proportion to the CPU frequency. So, if >> the CPUs are running at their max frequency, the device runs at its >> max frequency. If the CPUs are running at their min frequency, the >> device runs at its min frequency. It is interpolated for frequencies >> in between. >> >> Tested-by: Chen-Yu Tsai <wenst@chromium.org> >> Tested-by: Johnson Wang <johnson.wang@mediatek.com> >> Signed-off-by: Saravana Kannan <skannan@codeaurora.org> >> [Sibi: Integrated cpu-freqmap governor into passive_governor] >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> [Chanwoo: Fix conflict with latest code and cleanup code] >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > > This patch landed in today's linux next-20220512 as commit 2ab415d4e4e6 > ("PM / devfreq: Add cpu based scaling support to passive governor"). > > It triggers the following NULL pointer dereference on Exynos based boards: > > exynos-bus: new bus device registered: soc:bus-leftbus (100000 KHz ~ > 200000 KHz) > exynos-bus: new bus device registered: soc:bus-rightbus (100000 KHz ~ > 200000 KHz) > exynos-bus: new bus device registered: soc:bus-display (160000 KHz ~ > 200000 KHz) > exynos-bus: new bus device registered: soc:bus-fsys (100000 KHz ~ 134000 > KHz) > exynos-bus: new bus device registered: soc:bus-peri ( 50000 KHz ~ 100000 > KHz) > exynos-bus: new bus device registered: soc:bus-mfc (100000 KHz ~ 200000 KHz) > 8<--- cut here --- > Unable to handle kernel NULL pointer dereference at virtual address 0000003c > [0000003c] *pgd=00000000 > Internal error: Oops: 5 [#1] PREEMPT SMP ARM > Modules linked in: > CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 5.18.0-rc6-next-20220512 #5014 > Hardware name: Samsung Exynos (Flattened Device Tree) > Workqueue: devfreq_wq devfreq_monitor > PC is at __mutex_lock+0x48/0x948 > LR is at lock_is_held_type+0x104/0x1a4 > pc : [<c0b93098>] lr : [<c0b8f2ec>] psr: 60000053 > sp : f0889dc0 ip : 600000d3 fp : c1dca624 > r10: c1dca44c r9 : 00000000 r8 : c1984104 > r7 : c1d7f000 r6 : 00000000 r5 : 00000001 r4 : 00000008 > r3 : 00000000 r2 : 00000000 r1 : 2de44000 r0 : 00000000 > Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 4000404a DAC: 00000051 > Register r0 information: NULL pointer > Register r1 information: non-paged memory > Register r2 information: NULL pointer > Register r3 information: NULL pointer > Register r4 information: non-paged memory > Register r5 information: non-paged memory > Register r6 information: NULL pointer > Register r7 information: slab task_struct start c1d7f000 pointer offset 0 > Register r8 information: non-slab/vmalloc memory > Register r9 information: NULL pointer > Register r10 information: slab kmalloc-2k start c1dca000 pointer offset > 1100 size 2048 > Register r11 information: slab kmalloc-2k start c1dca000 pointer offset > 1572 size 2048 > Register r12 information: non-paged memory > Process kworker/u8:0 (pid: 8, stack limit = 0x(ptrval)) > Stack: (0xf0889dc0 to 0xf088a000) > ... > __mutex_lock from mutex_lock_nested+0x1c/0x24 > mutex_lock_nested from devfreq_passive_notifier_call+0x24/0x90 > devfreq_passive_notifier_call from srcu_notifier_call_chain+0x98/0x114 > srcu_notifier_call_chain from devfreq_set_target+0x6c/0x304 > devfreq_set_target from devfreq_update_target+0x98/0xe8 > devfreq_update_target from devfreq_monitor+0x28/0x1c0 > devfreq_monitor from process_one_work+0x288/0x774 > process_one_work from worker_thread+0x44/0x504 > worker_thread from kthread+0xf4/0x128 > kthread from ret_from_fork+0x14/0x2c > Exception stack(0xf0889fb0 to 0xf0889ff8) > ... > ---[ end trace 0000000000000000 ]--- > > The issue is caused by the lack of setting devfreq_passive_data->this > pointer in devfreq_passive_register_notifier. However, after adding: > > @@ -395,6 +395,9 @@ static int devfreq_passive_register_notifier(struct > devfreq *devfreq) > if (!parent) > return -EPROBE_DEFER; > > + if (!p_data->this) > + p_data->this = devfreq; > + > nb->notifier_call = devfreq_passive_notifier_call; > return devfreq_register_notifier(parent, nb, > DEVFREQ_TRANSITION_NOTIFIER); > } > > the NULL pointer dereference is gone, but I see the following warnings > on Odroid U3 board, which were not present before this patch: > > devfreq soc:bus-acp: failed to update devfreq using passive governor > devfreq soc:bus-c2c: failed to update devfreq using passive governor > devfreq soc:bus-acp: failed to update devfreq using passive governor > devfreq soc:bus-c2c: failed to update devfreq using passive governor Hi Marek, Thanks for the report. I'll fix it.
The devfreq passive governor has already supported the devfreq parent device for coupling the frequency change if some hardware have the constraints such as power sharing and so on. Add cpu based scaling support to passive governor with required-opp property. It uses the cpufreq notifier to catch the frequency change timing of cpufreq and get the next frequency according to new cpu frequency by using required-opp property. It is based on patch[1] and then just code clean-up by myself. Make the common code for both passive_devfreq and passive_cpufreq parent type to remove the duplicate code. [1] [RFC,v2] PM / devfreq: Add cpu based scaling support to passive_governor - https://lore.kernel.org/patchwork/patch/1101049/ Changes from v3: : ttps://patchwork.kernel.org/project/linux-pm/cover/20220509120337.92472-1-cw00.choi@samsung.com/ - Add tested-by tag of both Chen-Yu Tsai and Johnson Wang - Fix some typo Changes from v2: : https://patchwork.kernel.org/project/linux-pm/cover/20220507150145.531864-1-cw00.choi@samsung.com/ - Drop the following patch ("PM / devfreq: passive: Update frequency when start governor") - Move p_data->this initialization into cpufreq_passive_regiser_notifier() Changes from v1: : https://patchwork.kernel.org/project/linux-pm/cover/20210617060546.26933-1-cw00.choi@samsung.com/ - Rename cpu_data variable to parent_cpu_data to avoid build fail - Use for_each_possible_cpu macro when register cpufreq transition notifier - Add missing exception handling when cpufreq_passive_register_notifier is failed - Keep cpufreq_policy for posible cpus instead of NR_CPU in order to avoid the memory waste when NR_CPU is too high. - Add reviewed-by tag of Matthias Kaehlcke for patch1 Chanwoo Choi (3): PM / devfreq: Export devfreq_get_freq_range symbol within devfreq PM / devfreq: passive: Reduce duplicate code when passive_devfreq case PM / devfreq: passive: Keep cpufreq_policy for possible cpus Saravana Kannan (1): PM / devfreq: Add cpu based scaling support to passive governor drivers/devfreq/devfreq.c | 20 +- drivers/devfreq/governor.h | 27 ++ drivers/devfreq/governor_passive.c | 400 ++++++++++++++++++++++++----- include/linux/devfreq.h | 17 +- 4 files changed, 387 insertions(+), 77 deletions(-)