Re: [PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Chen Yu]

Hi,
On Sat, Feb 18, 2017 at 10:02:07AM +0100, Pavel Machek wrote:
> On Fri 2017-02-17 16:27:30, Chen Yu wrote:
> > Previously a bug was reported that on certain Broadwell
> > platform, after resumed from S3, the CPU is running at
> > an anomalously low speed, due to the BIOS has enabled the
> > MSR throttling across S3. The solution to this was to introduce
> > a quirk framework to save/restore tstate MSR register around
> > suspend/resume, in Commit 7a9c2dd08ead ("x86/pm:
> > Introduce quirk framework to save/restore extra MSR
> > registers around suspend/resume").
> > 
> > However there are still three problems left:
> > 1. More and more reports show that other platforms also
> >    encountered the same issue, so the quirk list might
> >    be endless.
> > 2. Each CPUs should take the save/restore operation into
> >    consideration, rather than the boot CPU alone.
> > 3. Normally ACPI T-state re-evaluation is done on resume,
> >    however there is no _TSS on the bogus platform, thus
> >    above re-evaluation code does not run on that machine.
> > 
> > Solution:
> > This patch is based on the fact that, we generally should not
> > expect the system to come back from resume with throttling
> > enabled, but leverage the OS components to deal with it,
> > such as thermal event. So we simply clear the MSR T-state
> > and print the warning if it is found to be enabled after
> > resumed back. Besides, we can remove the quirk in previous patch
> > later.
> 
> What if the machine _is_ hot? 
> 
Later the linux has a chance to adjust the tstate if the system is too hot,
with the help of thermal framework.
But if the cpu is not inside any thermal zone, then there is no way for the
OS to adjust the tstate after resume, however in this case I think it is up
to the user space to adjust the tstate msr, for example, by using thermald
daemon to bind the cpu to the thermal zone.

> > +static int acpi_throttling_init_ops(void)
> > +{
> > +	/*
> > +	 * Reevaluate on boot CPU. Since it is not always CPU0,
> > +	 * we can not invoke throttling_msr_reevaluate(0) directly.
> > +	 */
> 
> Boot cpu is not cpu#0? How can that be?
> 
In disable_nonboot_cpus, it first tries to disable cpus other than cpu#0,
but if cpu#0 is found to be offline, it will try to bypass it and use another
cpu as the boot/primary cpu:

if (!cpu_online(primary))
	primary = cpumask_first(cpu_online_mask);

> Should we introduce generic framework to "fix" all the cpus? Actually,
> should this be done right on cpu hotplug?
Do you mean, fix other MSR-inconsistent issues, not only the tstate MSR?
Currently the tstate re-adjusting is invoked in cpuhotplug notifier
after each nonboot cpus are brought up:
acpi_soft_cpu_online -> acpi_processor_reevaluate_tstate -> adjust_tstate_msr

Thanks,
Yu
> 
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html