Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs
From: Yu Chen
Date: Wed Feb 13 2019 - 11:44:21 EST
Hi Rafael,
On Mon, Feb 11, 2019 at 11:41:26AM +0100, Rafael J. Wysocki wrote:
> On Sat, Feb 9, 2019 at 12:54 PM Chen Yu <yu.c.chen@xxxxxxxxx> wrote:
> >
> > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > the power source changes), the maximum frequency of the
> > CPU is not updated accordingly. This is due to the policy's
> > cpuinfo.max is not updated when _PPC notifier fires.
> >
> > Fix this problem by updating the policy's cpuinfo.max
> > and broadcast the _PPC notifier to all online CPUs.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@xxxxxxxxx>
> > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> > ---
> > drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> > drivers/cpufreq/cpufreq.c | 2 ++
> > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++-
> > 3 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > index a303fd0e108c..737dbf5aa7f7 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> > "limited by BIOS, this should help");
> >
> > +static int broadcast_ppc;
> > +module_param(broadcast_ppc, int, 0644);
> > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > +
> > #define PPC_REGISTERED 1
> > #define PPC_IN_USE 2
> >
> > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> > else
> > acpi_processor_ppc_ost(pr->handle, 0);
> > }
> > - if (ret >= 0)
> > - cpufreq_update_policy(pr->id);
> > + if (ret >= 0) {
> > + if (broadcast_ppc) {
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu)
> > + cpufreq_update_policy(cpu);
> > + } else {
> > + cpufreq_update_policy(pr->id);
> > + }
> > + }
> > }
> >
> > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index e35a886e00bc..95e08816b512 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> >
> > policy->min = new_policy->min;
> > policy->max = new_policy->max;
> > + policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > + policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> > trace_cpu_frequency_limits(policy);
> >
> > policy->cached_target_freq = UINT_MAX;
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index dd66decf2087..e1881313c396 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> >
> > static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> > {
> > + int max_freq;
> > struct cpudata *cpu = all_cpu_data[policy->cpu];
> >
> > update_turbo_state();
>
> Well, update_turbo_state() should handle the case at hand already.
>
> That's what it's for actually: It checks if
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets
> global.turbo_disabled is that's the case.
>
> Why isn't that sufficient?
>
update_turbo_state() changes the flag of global.turbo_diabled but we
need to also leverage it to adjust the policy.max accordingly. This is why
we add intel_pstate_get_max_freq() to get the updated max freq in
intel_pstate_verify_policy().
> > + max_freq = intel_pstate_get_max_freq(cpu);
> > +
> > + if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > + max_freq != policy->cpuinfo.max_freq) {
> > + /*
> > + * System was not running under any constraints, but the
> > + * current max possible frequency is changed. So reset
> > + * policy limits.
> > + */
> > + policy->cpuinfo.max_freq = policy->max = max_freq;
> > + }
>
> Why does policy->cpuinfo.max_freq need to be updated?
>
This is my understanding:
There's a corner case that, if the system boots with battery,
the max cpu frequency will not scale up if we plug the AC later.
According to the log provided by Gabriele Mazzotta, if the system
boot up with battery, then plug the AC after boot up, the max perf ratio
and policy->cpuinfo.max will remain 17 rather than increasing to
30(when AC plugged thus turbo enabled):
[ 52.158810] CPU 0: _PPC is 6 - frequency limited
[ 52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000
[ 52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17
[ 52.158827] intel_pstate: cpu:0 global_min:8 global_max:30
[ 52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8
This is caused by:
In current intel_pstate, there's only one chance for policy.cpuinfo.max to get updated
which is during boot up in __intel_pstate_cpu_init(). If the turbo status changes,
we might need to also update the policy->cpuinfo.max to tell user that the hardware
status has changed.
So we give it a chance to adjust the policy.cpuinfo.max and policy.max in
cpufreq_driver->verify() according to turbo status, this is what this patch mainly
aims to do.
Besides, since on this platform there's only one _PPC notification for one CPU, it is
necessary to broadcast the notification to all CPUs on this package. And this patch
broadcast it to all online CPUs to make the change simpler.
Best,
Yu