Re: [PATCH] cpufreq,cppc: fix issue when hotplugging out policy->cpu

From: Viresh Kumar
Date: Fri Sep 04 2020 - 01:06:21 EST

Next message: John Fastabend: "Re: Packet gets stuck in NOLOCK pfifo_fast qdisc"
Previous message: Moshe Shemesh: "Re: [PATCH net-next RFC v3 02/14] devlink: Add reload actions counters"
In reply to: Ionela Voinescu: "[PATCH] cpufreq,cppc: fix issue when hotplugging out policy->cpu"
Next in thread: Ionela Voinescu: "Re: [PATCH] cpufreq,cppc: fix issue when hotplugging out policy->cpu"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 03-09-20, 12:19, Ionela Voinescu wrote:
> An issue is observed in the cpufreq CPPC driver when having dependency
> domains (PSD) and the policy->cpu is hotplugged out.
>
> Considering a platform with 4 CPUs and 2 PSD domains (CPUs 0 and 1 in
> PSD-1, CPUs 2 and 3 in PSD-2), cppc_cpufreq_cpu_init() will be called
> for the two cpufreq policies that are created and it will set
> all_cpu_data[policy->cpu]->cpu = policy->cpu.
>
> Therefore all_cpu_data[0]->cpu=0, and all_cpu_data[2]->cpu=2. But for
> CPUs 1 and 3, all_cpu_data[{1,3}]->cpu will remain 0 from the structure
> allocation.
>
> If CPU 2 is hotplugged out, CPU 3 will become policy->cpu. But its
> all_cpu_data[3]->cpu will remain 0. Later, when the .target() function
> is called for policy2, the cpu argument to cppc_set_perf() will be 0 and
> therefore it will use the performance controls of CPU 0, which will
> result in a performance level change for the wrong domain.
>
> While the possibility of setting a correct CPU value in the per-cpu
> cppc_cpudata structure is available, it can be noticed that this cpu value
> is not used at all outside the .target() function, where it's not actually
> necessary. Therefore, remove the cpu variable from the cppc_cpudata
> structure and use policy->cpu in the .target() function as done for the
> other CPPC cpufreq functions.
>
> Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with CPPC")
> Signed-off-by: Ionela Voinescu <ionela.voinescu@xxxxxxx>
> Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>
> Testing was done on a Juno R2 platform (with the proper ACPI/CPPC setup):
> CPUs 0, 1, 2, 3 are in PSD-0 (policy0), CPUs 4 and 5 are in PSD-4
> (policy4).
>
> Before the fix:
>
> root@sqwt-ubuntu:~# dmesg | grep address
> [ 2.165177] ACPI CPPC: ACPI desired perf address 0: - ffff80001004d200
> [ 2.174226] ACPI CPPC: ACPI desired perf address 1: - ffff800010055200
> [ 2.183231] ACPI CPPC: ACPI desired perf address 2: - ffff80001005d200
> [ 2.192234] ACPI CPPC: ACPI desired perf address 3: - ffff800010065200
> [ 2.201245] ACPI CPPC: ACPI desired perf address 4: - ffff80001006d218
> [ 2.210256] ACPI CPPC: ACPI desired perf address 5: - ffff800011ff1218
> [..]
> [ 2.801940] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 38300
> [ 2.835286] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 102400
> [..]
> root@sqwt-ubuntu:~# cd /sys/devices/system/cpu/cpufreq/
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed
> [ 72.098758] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 51200
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed
> [ 85.430645] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 102400
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 0 > ../cpu4/online
> [ 102.606380] CPPC Cpufreq:CPPC: Calculate: (6285/261)*4266=102727.
> [ 102.612491] CPPC Cpufreq:CPPC: Core rate = 1203832, arch timer rate: 50000000
> [ 102.619659] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 102400
> [ 102.626898] CPU4: shutdown
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed
> [ 141.116882] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 51200
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed
> [ 159.288273] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 102400
>
>
> After the fix:
>
> root@sqwt-ubuntu:~# cd /sys/devices/system/cpu/cpufreq/
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed
> [ 139.903322] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 51200
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed
> [ 147.279040] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 102400
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 0 > ../cpu4/online
> [ 153.598686] CPPC Cpufreq:CPPC: Calculate: (6171/253)*4266=104053.
> [ 153.604797] CPPC Cpufreq:CPPC: Core rate = 1219371, arch timer rate: 50000000
> [ 153.611960] ACPI CPPC: Writing to address for CPU 5:ffff800011ff1218: 102400
> [ 153.619190] CPU4: shutdown
> [ 153.621911] psci: CPU4 killed (polled 0 ms)
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed
> [ 170.122495] ACPI CPPC: Writing to address for CPU 5:ffff800011ff1218: 51200
> root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed
> [ 177.206342] ACPI CPPC: Writing to address for CPU 5:ffff800011ff1218: 102400
>
> Thanks,
> Ionela.
>
> drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
> include/acpi/cppc_acpi.h | 1 -
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index f29e8d0553a8..54457f5fe49e 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -149,8 +149,9 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
> unsigned int target_freq,
> unsigned int relation)
> {
> - struct cppc_cpudata *cpu;
> struct cpufreq_freqs freqs;
> + int cpu_num = policy->cpu;
> + struct cppc_cpudata *cpu;
> u32 desired_perf;
> int ret = 0;
>
> @@ -166,12 +167,12 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
> freqs.new = target_freq;
>
> cpufreq_freq_transition_begin(policy, &freqs);
> - ret = cppc_set_perf(cpu->cpu, &cpu->perf_ctrls);
> + ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
> cpufreq_freq_transition_end(policy, &freqs, ret != 0);
>
> if (ret)
> pr_debug("Failed to set target on CPU:%d. ret:%d\n",
> - cpu->cpu, ret);
> + cpu_num, ret);
>
> return ret;
> }
> @@ -247,7 +248,6 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>
> cpu = all_cpu_data[policy->cpu];
>
> - cpu->cpu = cpu_num;
> ret = cppc_get_perf_caps(policy->cpu, &cpu->perf_caps);
>
> if (ret) {
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index a6a9373ab863..451132ec83c9 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -124,7 +124,6 @@ struct cppc_perf_fb_ctrs {
>
> /* Per CPU container for runtime CPPC management. */
> struct cppc_cpudata {
> - int cpu;
> struct cppc_perf_caps perf_caps;
> struct cppc_perf_ctrls perf_ctrls;
> struct cppc_perf_fb_ctrs perf_fb_ctrs;

With the way things are designed, I believe this is one of the bugs
out of many.

The structure cppc_cpudata must be shared across all CPUs of the same
policy, so they all end up using the same set of values for different
variables. i.e. it shouldn't be a per-cpu thing at all. Just allocate
it from cpufreq_driver->init and store in policy->driver_data for use
elsewhere.

That would be a proper fix IMO, we just avoided one of the bugs here
otherwise.

--
viresh

Next message: John Fastabend: "Re: Packet gets stuck in NOLOCK pfifo_fast qdisc"
Previous message: Moshe Shemesh: "Re: [PATCH net-next RFC v3 02/14] devlink: Add reload actions counters"
In reply to: Ionela Voinescu: "[PATCH] cpufreq,cppc: fix issue when hotplugging out policy->cpu"
Next in thread: Ionela Voinescu: "Re: [PATCH] cpufreq,cppc: fix issue when hotplugging out policy->cpu"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]