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@xxxxxxxxxxxxxxx
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> 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(-)
>
[..]
> +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.
