Re: [PATCH 1/2][RFC v2] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC

[Yu Chen]

On Fri, Mar 01, 2019 at 10:34:46AM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 28, 2019 at 11:18 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >
> > On Thu, Feb 28, 2019 at 6:59 PM Chen Yu <yu.c.chen@xxxxxxxxx> wrote:
> > >
> > > On some problematic platforms, the _PPC notifier is
> > > only delivered to one CPU, which might cause information
> > > inconsistent between CPUs within the package. Thus introduce a boot up parameter to broadcast this _PPC notifier onto all
> > > online CPUs.
> > >
> > > Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> > > ---
> > >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 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);
> >
> > This doesn't actually help AFAICS, because it only causes
> > acpi_processor_ppc_notifier() to be called for all policies, but
> > pr->performance_platform_limit is re-computed for the target CPU only
> > anyway, so the limit will only be applied to that one.
> >
> > What happens in the BZ is that invoking cpufreq_update_policy() for
> > all CPUs causes ->verify() to run on all of them which triggers
> > update_turbo_state() and cpuinfo.max_freq update, because
> > global.turbo_disabled has changed.
> >
> > That is rather less than straightforward and intel_pstate really
> > doesn't need any _PPC change notifications to notice that the
> > MSR_IA32_MISC_ENABLE_TURBO_DISABLE bit has changed as it checks that
> > bit on every P-state update.
> 
> Which admittedly may not be necessary if notifications are delivered.
> 
> I still don't think that updating all policies from
> acpi_processor_ppc_has_changed() is a good idea, but yes, there is a
> problem that if MSR_IA32_MISC_ENABLE_TURBO_DISABLE goes from set to
> unset, all policies need to be updated to update policy->max
> accordingly, 
Agree. policy->max might needed to be updated otherwise we might
not get correct cpufreq limit range. According to the report log
from bugzilla, if the system boots without AC and then plug the AC
after boot up, the max_perf_ratio would be incorrect because policy->max
is not updated.

# Plug the AC:

[   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

Thanks,
Yu
> so looking at MSR_IA32_MISC_ENABLE_TURBO_DISABLE from
> within the driver without triggering a policy update is not
> sufficient.