| Message ID | ee0250bfc7709ce0a0cc93bf83336133d5e248c4.1551375162.git.yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Update the cpuinfo.max when power supply changes | expand |
On Thu, Feb 28, 2019 at 6:59 PM Chen Yu <yu.c.chen@intel.com> 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@intel.com> > --- > 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. So no, we are not going to do this to fix the particular issue at hand. > + } else { > + cpufreq_update_policy(pr->id); > + } > + } > } > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > -- > 2.17.1 >
On Thu, Feb 28, 2019 at 11:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Feb 28, 2019 at 6:59 PM Chen Yu <yu.c.chen@intel.com> 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@intel.com> > > --- > > 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, so looking at MSR_IA32_MISC_ENABLE_TURBO_DISABLE from within the driver without triggering a policy update is not sufficient.
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@kernel.org> wrote: > > > > On Thu, Feb 28, 2019 at 6:59 PM Chen Yu <yu.c.chen@intel.com> 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@intel.com> > > > --- > > > 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.
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)
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@intel.com> --- drivers/acpi/processor_perflib.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)