Re: [PATCH V2 3/3] cpufreq: CPPC: Add support for frequency invariance
From: Ionela Voinescu <hidden>
Date: 2021-06-16 12:48:13
Also in:
linux-acpi, lkml
Hi, I was looking forward to the complete removal of stop_cpu() :). On Wednesday 16 Jun 2021 at 12:18:09 (+0530), Viresh Kumar wrote:
The Frequency Invariance Engine (FIE) is providing a frequency scaling correction factor that helps achieve more accurate load-tracking. Normally, this scaling factor can be obtained directly with the help of the cpufreq drivers as they know the exact frequency the hardware is running at. But that isn't the case for CPPC cpufreq driver. Another way of obtaining that is using the arch specific counter support, which is already present in kernel, but that hardware is optional for platforms. This patch updates the CPPC driver to register itself with the topology core to provide its own implementation (cppc_scale_freq_tick()) of topology_scale_freq_tick() which gets called by the scheduler on every tick. Note that the arch specific counters have higher priority than CPPC counters, if available, though the CPPC driver doesn't need to have any special handling for that. On an invocation of cppc_scale_freq_tick(), we schedule an irq work (since we reach here from hard-irq context), which then schedules a normal work item and cppc_scale_freq_workfn() updates the per_cpu arch_freq_scale variable based on the counter updates since the last tick. To allow platforms to disable this CPPC counter-based frequency invariance support, this is all done under CONFIG_ACPI_CPPC_CPUFREQ_FIE, which is enabled by default. This also exports sched_setattr_nocheck() as the CPPC driver can be built as a module. Cc: linux-acpi@vger.kernel.org Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/Kconfig.arm | 10 ++ drivers/cpufreq/cppc_cpufreq.c | 232 ++++++++++++++++++++++++++++++--- include/linux/arch_topology.h | 1 + kernel/sched/core.c | 1 + 4 files changed, 227 insertions(+), 17 deletions(-)
[..]
quoted hunk ↗ jump to hunk
+static void cppc_cpufreq_start_cpu(struct cpufreq_policy *policy, + unsigned int cpu) +{ + struct cppc_freq_invariance *cppc_fi = &per_cpu(cppc_freq_inv, cpu); + int ret; + + cppc_fi->cpu = cpu; + cppc_fi->cpu_data = policy->driver_data; + kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn); + init_irq_work(&cppc_fi->irq_work, cppc_irq_work); + + ret = cppc_get_perf_ctrs(cpu, &cppc_fi->prev_perf_fb_ctrs); + if (ret) { + pr_warn("%s: failed to read perf counters: %d\n", __func__, + ret); + return; + } + + /* Register for freq-invariance */ + topology_set_scale_freq_source(&cppc_sftd, cpumask_of(cpu)); +} + +static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy, + unsigned int cpu) +{ + struct cppc_freq_invariance *cppc_fi = &per_cpu(cppc_freq_inv, cpu); + + topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, cpumask_of(cpu)); + + irq_work_sync(&cppc_fi->irq_work); + kthread_cancel_work_sync(&cppc_fi->work); +}
I'll only comment on this for now as I should know the rest. Let's assume we don't have these, what happens now is the following: 1. We hotplug out the last CPU in a policy, we call the .stop_cpu()/exit() function which will free the cppc_cpudata structure. The only vulnerability is if we have a last tick on that last CPU, after the above callback was called. 2. When the CPU at 1. gets hotplugged back in, the cppc_fi->cpu_data is stale. We do not have a problem when removing the CPPC cpufreq module as we're doing cppc_freq_invariance_exit() before unregistering the driver and freeing the data. Are 1. and 2 the only problems we have, or have I missed any? Thanks, Ionela.